Cordy/Cairn

Fork 0

WebDAV: MOVE with Overwrite: T deletes the destination before renaming (not atomic) #496

New issue

Closed

opened 2026-09-11 21:04:03 +00:00 by Cordy · 1 comment

Cordy commented

2026-09-11 21:04:03 +00:00

Owner

Status: from source (read at bd006ef, not reproduced). Unchanged on main (e1c855d).

What happens

In the MOVE/COPY handler (internal/dav/dav.go:292–336), when the destination exists and Overwrite is not F, the server DELETEs the destination and then renames the source onto it. If the rename then fails — no space, permissions, a crash or restart between the two steps — the destination is already gone and the source has not arrived.

Impact

A rename that overwrites an existing file (which sync clients issue when a local rename lands on an already-synced name) can lose the overwritten file on failure, with no copy anywhere.

Suggested fix

For files, rename over the destination directly — os.Rename replaces the target atomically on POSIX (and MoveFileEx with MOVEFILE_REPLACE_EXISTING on Windows). For collections, stage: rename the destination aside, rename the source in, then remove the staged copy, restoring it if the second step fails. Add a fault-injection test.

Found while building the cairn-desktop WebDAV client (phase 2, Cordy/cairn-desktop#23). Filed by Claude on behalf of @Cordy.

**Status: from source** (read at `bd006ef`, not reproduced). Unchanged on `main` (`e1c855d`). ## What happens In the MOVE/COPY handler (`internal/dav/dav.go:292–336`), when the destination exists and `Overwrite` is not `F`, the server **DELETEs the destination and then renames** the source onto it. If the rename then fails — no space, permissions, a crash or restart between the two steps — the destination is already gone and the source has not arrived. ## Impact A rename that overwrites an existing file (which sync clients issue when a local rename lands on an already-synced name) can lose the overwritten file on failure, with no copy anywhere. ## Suggested fix For files, rename over the destination directly — `os.Rename` replaces the target atomically on POSIX (and `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING` on Windows). For collections, stage: rename the destination aside, rename the source in, then remove the staged copy, restoring it if the second step fails. Add a fault-injection test. --- Found while building the cairn-desktop WebDAV client (phase 2, Cordy/cairn-desktop#23). Filed by Claude on behalf of @Cordy.

Cordy added this to the v0.7 — Closed beta (dogfood + hardening) milestone

2026-09-11 21:05:56 +00:00

Cordy referenced this issue from a commit

2026-09-12 15:54:37 +00:00

one-shot: #495 hidden temps + #496 atomic overwrite (red-then-green)

forgejo-actions referenced this issue from a commit

2026-09-12 15:59:26 +00:00

#495 #496: hide server-internal temp names; overwriting MOVE/COPY is atomic with staged restore

Cordy referenced this issue

2026-09-12 16:01:40 +00:00

#495 #496: hide server-internal temp names; atomic overwriting MOVE/COPY #504

Cordy referenced this issue from a commit

2026-09-12 16:03:52 +00:00

Merge pull request '#495 #496: hide server-internal temp names; atomic overwriting MOVE/COPY' (#504) from fix-495-496 into main

Cordy referenced this issue

2026-09-12 16:35:43 +00:00

WebDAV: in-flight upload temp files (.cairn-upload-*) appear in PROPFIND listings #495

Cordy commented

2026-09-12 16:35:46 +00:00

Author

Owner

Shipped in v0.6.169, live on both dogfoods.

What landed in moveCopy:

Source is Stat'd first (clean 404 early), destination Stat'd for the Overwrite: F → 412 check.
When the destination exists and either side is a directory, the destination is renamed aside to <dst>.cairn-replace-<token> before the operation: on success the staged copy is deleted; on failure it is restored, so a failed MOVE can no longer leave the destination half-deleted or gone.
If the restore itself fails, the response is a loud 500 naming the staged path — data is never silently dropped.
Staging names are hidden from all read paths per #495, so the window is invisible to clients.

TDD: move_test.go injects rename/copy failures via a failOp driver wrapper and asserts the destination survives — witnessed red first.

Shipped in **v0.6.169**, live on both dogfoods. What landed in `moveCopy`: - Source is Stat'd first (clean 404 early), destination Stat'd for the `Overwrite: F` → 412 check. - When the destination exists and either side is a directory, the destination is renamed aside to `<dst>.cairn-replace-<token>` **before** the operation: on success the staged copy is deleted; on failure it is restored, so a failed MOVE can no longer leave the destination half-deleted or gone. - If the restore itself fails, the response is a loud 500 naming the staged path — data is never silently dropped. - Staging names are hidden from all read paths per #495, so the window is invisible to clients. TDD: `move_test.go` injects rename/copy failures via a `failOp` driver wrapper and asserts the destination survives — witnessed red first.

Cordy closed this issue

2026-09-12 16:35:52 +00:00

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Cordy/Cairn#496

No description provided.

Rows
Columns