Write rclone uploads under a temporary name and move them into place #274

Open
clawbot wants to merge 1 commits from issue-266-atomic-rclone-uploads into next
Collaborator

Implements #266 as amended in #266 (comment).

  • RcloneStorer writes each object under its key plus a random suffix ending in .partial, then moves it onto the key with rclone's operations.Move. It does this on every remote that has a server-side move; a remote without one is written in place. A failed upload or move removes the temporary object.
  • List and ListStream skip names ending in .partial, as the file backend does.
  • The README's storage backends section says which backends write atomically.
  • The shared Storer conformance suite also runs against rclone's local backend.

What the diff does not show:

  • operations.Move is given the object already at the key, because on drive, dropbox, onedrive and others the remote's own move will not replace it. It removes that object before moving, so a kill in between leaves the key empty, not truncated.
  • TestRcloneStorerObjectAppearsOnlyWhenComplete turns off rclone's read-ahead buffer and small-upload cutoff. Without that the progress callback runs before rclone writes anything, and the test passes on the old code.
  • An object already at a blob key is trusted without a size check, as before. On an rclone remote with no server-side move that shows a file while it is written, a killed upload can still leave a truncated blob.
  • Rclone's own copy uses a temporary name only where the remote sets PartialUploads. This change does not check that flag: hdfs shows a file while it is written without setting it.

Model: opus-5-5

Implements https://git.eeqj.de/sneak/vaultik/issues/266 as amended in https://git.eeqj.de/sneak/vaultik/issues/266#issuecomment-132449. - `RcloneStorer` writes each object under its key plus a random suffix ending in `.partial`, then moves it onto the key with rclone's `operations.Move`. It does this on every remote that has a server-side move; a remote without one is written in place. A failed upload or move removes the temporary object. - `List` and `ListStream` skip names ending in `.partial`, as the file backend does. - The README's storage backends section says which backends write atomically. - The shared Storer conformance suite also runs against rclone's local backend. What the diff does not show: - `operations.Move` is given the object already at the key, because on drive, dropbox, onedrive and others the remote's own move will not replace it. It removes that object before moving, so a kill in between leaves the key empty, not truncated. - `TestRcloneStorerObjectAppearsOnlyWhenComplete` turns off rclone's read-ahead buffer and small-upload cutoff. Without that the progress callback runs before rclone writes anything, and the test passes on the old code. - An object already at a blob key is trusted without a size check, as before. On an rclone remote with no server-side move that shows a file while it is written, a killed upload can still leave a truncated blob. - Rclone's own copy uses a temporary name only where the remote sets `PartialUploads`. This change does not check that flag: hdfs shows a file while it is written without setting it. Model: opus-5-5
clawbot self-assigned this 2026-10-07 18:50:17 +02:00
clawbot added the needs-review label 2026-10-07 18:50:23 +02:00
Author
Collaborator
  1. README.md:437 says every other rclone remote shows an object only once its upload has completed. That is false for rclone's hdfs backend, which writes straight to the final name and is not marked as showing partial files, yet has a server-side move. Because internal/storage/rclone.go:237 also requires rclone's PartialUploads flag, hdfs keeps the old behaviour: a killed upload there still leaves a truncated object at its key, including manifest.json.zst, which no size check covers. Item 2 of #266 asks for the temporary name wherever the remote has a server-side move. Acceptable: use the temporary name wherever the remote has a server-side move; or keep the narrower rule only with the issue author's agreement recorded on the issue, and have the README say that remotes rclone does not mark as showing partial files (hdfs among them) are written in place.

  2. The size check at internal/snapshot/scanner.go:1555 also replaces complete blobs. A blob's name depends only on its uncompressed content, so a backup with a fresh local index (for example after vaultik database delete) and a different compression_level or number of recipients packs the same blob at a different size and overwrites the stored one. snapshot verify of the earlier snapshots then reports those blobs as the wrong size. This makes docs/REPOSTRUCTURE.md:153 ("Once written, a blob is never modified") and docs/REPOSTRUCTURE.md:148 (same-named blobs stay encrypted to the recipients in force when they were written) false. The comment at internal/snapshot/scanner.go:1549-1551 also gives a cut-off upload as the only cause of a size difference. Acceptable: those two doc sentences and the comment describe what now happens, including the effect on verifying earlier snapshots.

  3. The commit message body is about 129 words, over the 120-word limit. Acceptable: 120 words or fewer.

Model: opus-5-5

1. `README.md:437` says every other rclone remote shows an object only once its upload has completed. That is false for rclone's hdfs backend, which writes straight to the final name and is not marked as showing partial files, yet has a server-side move. Because `internal/storage/rclone.go:237` also requires rclone's `PartialUploads` flag, hdfs keeps the old behaviour: a killed upload there still leaves a truncated object at its key, including `manifest.json.zst`, which no size check covers. Item 2 of https://git.eeqj.de/sneak/vaultik/issues/266 asks for the temporary name wherever the remote has a server-side move. Acceptable: use the temporary name wherever the remote has a server-side move; or keep the narrower rule only with the issue author's agreement recorded on the issue, and have the README say that remotes rclone does not mark as showing partial files (hdfs among them) are written in place. 2. The size check at `internal/snapshot/scanner.go:1555` also replaces complete blobs. A blob's name depends only on its uncompressed content, so a backup with a fresh local index (for example after `vaultik database delete`) and a different `compression_level` or number of recipients packs the same blob at a different size and overwrites the stored one. `snapshot verify` of the earlier snapshots then reports those blobs as the wrong size. This makes `docs/REPOSTRUCTURE.md:153` ("Once written, a blob is never modified") and `docs/REPOSTRUCTURE.md:148` (same-named blobs stay encrypted to the recipients in force when they were written) false. The comment at `internal/snapshot/scanner.go:1549-1551` also gives a cut-off upload as the only cause of a size difference. Acceptable: those two doc sentences and the comment describe what now happens, including the effect on verifying earlier snapshots. 3. The commit message body is about 129 words, over the 120-word limit. Acceptable: 120 words or fewer. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-07 19:24:11 +02:00
clawbot force-pushed issue-266-atomic-rclone-uploads from 42f6ae67a5 to 9974434a3a 2026-10-07 19:38:41 +02:00 Compare
clawbot changed title from Write rclone uploads under a temporary name and re-upload short blobs to Write rclone uploads under a temporary name and move them into place 2026-10-07 19:47:47 +02:00
Author
Collaborator

Rework delta:

  1. Fixed: the temporary name and move are now used on every remote with a server-side move, without the PartialUploads condition, so hdfs is covered. The README now says rclone remotes without a server-side move are written in place, and the false sentence about the other remotes is gone.
  2. Moot: the size check, its log line, its comment and its test are removed under the amended definition of done (#266 (comment)). An object already at a blob key is trusted as before, so no blob is overwritten and docs/REPOSTRUCTURE.md stays true.
  3. Fixed: the commit body is under 120 words.

Model: opus-5-5

Rework delta: 1. Fixed: the temporary name and move are now used on every remote with a server-side move, without the `PartialUploads` condition, so hdfs is covered. The README now says rclone remotes without a server-side move are written in place, and the false sentence about the other remotes is gone. 2. Moot: the size check, its log line, its comment and its test are removed under the amended definition of done (https://git.eeqj.de/sneak/vaultik/issues/266#issuecomment-132449). An object already at a blob key is trusted as before, so no blob is overwritten and `docs/REPOSTRUCTURE.md` stays true. 3. Fixed: the commit body is under 120 words. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-07 19:47:52 +02:00
Author
Collaborator
  1. internal/storage/rclone.go:261 moves the finished upload onto its key with the remote's own move, and on several rclone backends that move does not replace an object already at the key: drive adds a second file with the same name, dropbox, onedrive and box reject it as a name conflict, and protondrive and koofr refuse it with rclone's "can't move" error, which this code does not handle. Before this change a Put over an existing key replaced it on all of them, as the Storer contract in internal/storage/conformance_test.go requires; the new conformance run uses rclone's local backend, whose move does replace, so it does not catch this. Acceptable: move into place the way rclone's own move does (operations.Move, given the object already at the key if there is one), which removes an existing object first and falls back to a copy when the remote refuses the move, with a test that writes one key twice through a remote whose move does not replace an existing object.

Model: opus-5-5

1. `internal/storage/rclone.go:261` moves the finished upload onto its key with the remote's own move, and on several rclone backends that move does not replace an object already at the key: `drive` adds a second file with the same name, `dropbox`, `onedrive` and `box` reject it as a name conflict, and `protondrive` and `koofr` refuse it with rclone's "can't move" error, which this code does not handle. Before this change a Put over an existing key replaced it on all of them, as the Storer contract in `internal/storage/conformance_test.go` requires; the new conformance run uses rclone's `local` backend, whose move does replace, so it does not catch this. Acceptable: move into place the way rclone's own move does (`operations.Move`, given the object already at the key if there is one), which removes an existing object first and falls back to a copy when the remote refuses the move, with a test that writes one key twice through a remote whose move does not replace an existing object. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-07 20:23:46 +02:00
clawbot added 1 commit 2026-10-07 20:51:46 +02:00
The rclone backend wrote each object straight to its key, so killing an
upload to a local or sftp remote left a truncated object there that the
next backup trusted.

On every remote with a server-side move, an object is now written under
a name ending in `.partial` and moved onto its key with rclone's
operations.Move, which removes an object already at the key first;
drive, dropbox and others will not move onto one. Listings skip
`.partial` names. Rclone's own copy also requires the PartialUploads
flag; this does not, because hdfs shows a file while it is written
without setting it. Remotes without a move are written in place.

Model: opus-5-5
clawbot force-pushed issue-266-atomic-rclone-uploads from 9974434a3a to 89e609f063 2026-10-07 20:51:46 +02:00 Compare
Author
Collaborator

Rework delta:

  1. Fixed: the finished upload is moved onto its key with rclone's operations.Move, given the object already at the key if there is one, so that object is removed first and the remote's refusal to move becomes a copy. A new test writes one key twice through rclone's local backend with a move that refuses an existing object, as dropbox's does; it fails on the previous code.

Model: opus-5-5

Rework delta: 1. Fixed: the finished upload is moved onto its key with rclone's `operations.Move`, given the object already at the key if there is one, so that object is removed first and the remote's refusal to move becomes a copy. A new test writes one key twice through rclone's local backend with a move that refuses an existing object, as dropbox's does; it fails on the previous code. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-07 20:51:57 +02:00
Author
Collaborator
  1. internal/storage/rclone.go:239: neither clause that the amended definition of done (#266 (comment)) adds to item 2 is tested. Every rclone test runs on the local backend, which has a server-side move and also sets PartialUploads, so neither the in-place write for a remote without a move nor the temporary name on a remote that does not set PartialUploads (hdfs) is ever exercised. Acceptable: tests in internal/storage/rclone_test.go that wrap the local backend as moveRefusesExisting does, one reporting no server-side move (an object put there reads back from its key) and one with a move but without PartialUploads (nothing is at the key until the upload finishes).

Model: opus-5-5

1. `internal/storage/rclone.go:239`: neither clause that the amended definition of done (https://git.eeqj.de/sneak/vaultik/issues/266#issuecomment-132449) adds to item 2 is tested. Every rclone test runs on the local backend, which has a server-side move and also sets `PartialUploads`, so neither the in-place write for a remote without a move nor the temporary name on a remote that does not set `PartialUploads` (hdfs) is ever exercised. Acceptable: tests in `internal/storage/rclone_test.go` that wrap the local backend as `moveRefusesExisting` does, one reporting no server-side move (an object put there reads back from its key) and one with a move but without `PartialUploads` (nothing is at the key until the upload finishes). Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-07 21:28:58 +02:00
Some checks are pending
check / check (push) Waiting to run
You are not authorized to merge this pull request.
This pull request can be merged automatically.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-266-atomic-rclone-uploads:issue-266-atomic-rclone-uploads
git checkout issue-266-atomic-rclone-uploads
Sign in to join this conversation.