This is a large, well-designed piece of work. The architectural shift — moving backup/restore from running-on-the-server shell commands to a proper RPC server so the operation works from any client — is the right call, and the implementation is careful throughout.
The core server (servers/disk/), the client commands (cli/commands/disk_{backup,restore,transfer}.go), the FindLoopByBacking correctness fix in diskio, the transfer/staging machinery, the undelete path, and the tests. The blackbox test suite is a good sanity check for the RFD claim — it proves no sudo or --data-path is needed.
Server has no mutex, and every RPC handler — Backup, Restore, TransferOffset, and Undelete — can arrive concurrently. The shared state is the filesystem under diskDataPath()/transfers/:
sweepTransfers runs inside stageForClient and receiveSnapshot. If two backup calls arrive for the same disk simultaneously (say, from two operators or a retry racing a stale connection), one could delete a .part file the other just created, or both could read loadStaging → "no metadata" → both proceed to os.OpenFile(path, O_CREATE|O_RDWR|O_TRUNC …) on the same transfer id, truncating each other's work.openTransfer is not atomic. Two concurrent uploads with the same id could both see have == offset, then both open the file for O_APPEND, producing interleaved writes. The character-validation guard on the transfer id prevents most accidental collisions, but it does not prevent two calls with the same id from racing.In practice this may rarely bite, because the client generates a fresh UUID per invocation and the retry loop is sequential. But the server itself has no guard, so a misbehaving client (or two concurrent operators backing up the same disk) has a real path to data corruption. A per-transfer-id lock or a sync.Map keyed on transfer id would close it.
sweepTransfers only cleans .part files, not .meta files. An orphaned .meta (with no matching .part) would survive every sweep, accumulating indefinitely. The sweep filter is strings.HasSuffix(e.Name(), ".part"), so .meta files are never touched by the TTL logic. discardStaging deletes both files, but the sweep only removes one. Over time, on a busy cluster, the transfers directory would accumulate metadata orphans. This should be cheap to fix: also glob and remove .meta files older than the TTL.
refuseLiveImage fails closed, correctly, but Restore calls it after PrepareRestore. If PrepareRestore creates a new disk entity and liveImageDevice then returns an error (not device-in-use, but a sysfs-read error), the cleanup deferred at line 52–66 of restore.go does fire, which is good. But the ordering could be worth a comment — the refusal lands after entity creation, so the rollback path is exercised on every mount-check failure, not just when the disk was already live.
orderingKeyTime silently returns the zero time on a parse failure. restorePointFromUpdate then passes the zero time to timestamp, which guards t.IsZero() and returns nil, so no timestamp is set. This is deliberately best-effort and tested. No issue.
The syncedSize seek in disk_backup.go:129. The comment explains why — writes must continue at the end — but the seek is unnecessary here: outFile was opened with os.Create and receives only appended writes (the server streams into it). The seek cannot hurt, but the reasoning in the comment ("writes go on at the end") is not obviously true for a client-side file opened at offset 0 for receiving; in practice io.Copy leaves the position at the end, so the seek is a no-op. No bug, but the comment is slightly misleading.
transferPath character-allow-list validation is correctly paranoid: path traversal is the attack vector when client-supplied names name files, and the test coverage is explicit (TestTransferPathRejectsIdsThatEscapeTheDirectory).FindLoopByBacking fix — checking !backing.Deleted — is exactly right. The comment explaining why (a renamed image's old inode stays attached but reports the new path, not the deleted marker) is the kind of explanation that saves the next reader.undelete rollback logic handles the cross-filesystem case (rename won't help if the holding area and the volumes dir are on different filesystems) — actually no, it does rely on os.Rename which would fail across filesystems. On a single data path that's fine, but worth keeping in mind if data directories ever span filesystems..part files (strings.HasSuffix(e.Name(), ".part")). A .meta file whose matching .part was deleted (for example, by discardStaging deleting the part file after the meta was written, but before the meta was also removed — or by a server crash between the two os.Remove calls in discardStaging) will survive every sweep indefinitely. The accumulation is likely small and harmless in practice, but it is a leak. Consider adding a second pass that removes .meta files older than the TTL whose matching .part is absent.openTransfer does a Stat to learn have, then opens the file — no lock between those two steps. Two concurrent RPC calls with the same transfer id (say, a misbehaving client or two retries racing a stale connection) could both see have == offset, both pass the check, and then both append to the file, interleaving their writes. The server has no mutex protecting this operation. A simple per-id mutex (or a sync.Map[string, *sync.Mutex]) acquired around the Stat+Open pair would close the race.Verdict: caveats
{
"owner": "mirendev",
"repo": "runtime",
"number": 1156,
"verdict": "caveats",
"event": "comment",
"summary": "## Review: Make disk backup and restore work from anywhere (RFD-108)\n\nThis is a large, well-designed piece of work. The architectural shift — moving backup/restore from running-on-the-server shell commands to a proper RPC server so the operation works from any client — is the right call, and the implementation is careful throughout.\n\n### What I looked at\n\nThe core server (`servers/disk/`), the client commands (`cli/commands/disk_{backup,restore,transfer}.go`), the `FindLoopByBacking` correctness fix in `diskio`, the transfer/staging machinery, the undelete path, and the tests. The blackbox test suite is a good sanity check for the RFD claim — it proves no `sudo` or `--data-path` is needed.\n\n### The one concrete concern: no concurrency protection on the transfer store\n\n`Server` has no mutex, and every RPC handler — `Backup`, `Restore`, `TransferOffset`, and `Undelete` — can arrive concurrently. The shared state is the filesystem under `diskDataPath()/transfers/`:\n\n- `sweepTransfers` runs inside `stageForClient` and `receiveSnapshot`. If two backup calls arrive for the same disk simultaneously (say, from two operators or a retry racing a stale connection), one could delete a `.part` file the other just created, or both could read `loadStaging` → \"no metadata\" → both proceed to `os.OpenFile(path, O_CREATE|O_RDWR|O_TRUNC …)` on the same transfer id, truncating each other's work.\n- The Stat/OpenFile sequence in `openTransfer` is not atomic. Two concurrent uploads with the same id could both see `have == offset`, then both open the file for `O_APPEND`, producing interleaved writes. The character-validation guard on the transfer id prevents most accidental collisions, but it does not prevent two calls with the _same_ id from racing.\n\nIn practice this may rarely bite, because the client generates a fresh UUID per invocation and the retry loop is sequential. But the server itself has no guard, so a misbehaving client (or two concurrent operators backing up the same disk) has a real path to data corruption. A per-transfer-id lock or a `sync.Map` keyed on transfer id would close it.\n\n### Other findings (not blockers)\n\n**`sweepTransfers` only cleans `.part` files, not `.meta` files.** An orphaned `.meta` (with no matching `.part`) would survive every sweep, accumulating indefinitely. The sweep filter is `strings.HasSuffix(e.Name(), \".part\")`, so `.meta` files are never touched by the TTL logic. `discardStaging` deletes both files, but the sweep only removes one. Over time, on a busy cluster, the transfers directory would accumulate metadata orphans. This should be cheap to fix: also glob and remove `.meta` files older than the TTL.\n\n**`refuseLiveImage` fails closed, correctly, but `Restore` calls it after `PrepareRestore`.** If `PrepareRestore` creates a new disk entity and `liveImageDevice` then returns an error (not device-in-use, but a sysfs-read error), the cleanup deferred at line 52–66 of `restore.go` does fire, which is good. But the ordering could be worth a comment — the refusal lands after entity creation, so the rollback path is exercised on every mount-check failure, not just when the disk was already live.\n\n**`orderingKeyTime` silently returns the zero time on a parse failure.** `restorePointFromUpdate` then passes the zero time to `timestamp`, which guards `t.IsZero()` and returns `nil`, so no timestamp is set. This is deliberately best-effort and tested. No issue.\n\n**The `syncedSize` seek in `disk_backup.go:129`.** The comment explains why — writes must continue at the end — but the seek is unnecessary here: `outFile` was opened with `os.Create` and receives only appended writes (the server streams into it). The seek cannot hurt, but the reasoning in the comment (\"writes go on at the end\") is not obviously true for a client-side file opened at offset 0 for receiving; in practice `io.Copy` leaves the position at the end, so the seek is a no-op. No bug, but the comment is slightly misleading.\n\n### What works well\n\n- The `transferPath` character-allow-list validation is correctly paranoid: path traversal is the attack vector when client-supplied names name files, and the test coverage is explicit (`TestTransferPathRejectsIdsThatEscapeTheDirectory`).\n- The `FindLoopByBacking` fix — checking `!backing.Deleted` — is exactly right. The comment explaining *why* (a renamed image's old inode stays attached but reports the new path, not the deleted marker) is the kind of explanation that saves the next reader.\n- The staging completion marker pattern (write metadata *last*, so a half-compressed file left by a crash has no marker and is treated as nothing) is a solid distributed-systems idiom, correctly applied.\n- The `undelete` rollback logic handles the cross-filesystem case (rename won't help if the holding area and the volumes dir are on different filesystems) — actually no, it *does* rely on `os.Rename` which would fail across filesystems. On a single data path that's fine, but worth keeping in mind if data directories ever span filesystems.\n- Test coverage is deep for the core server logic (unit tests cover resume, staging, transfer-path validation, undelete rollback on missing image, and the refusal/interrupted upload distinction). The blackbox test covers the end-to-end RPC claim.",
"comments": [
{
"path": "servers/disk/transfer.go",
"line": 238,
"side": "RIGHT",
"body": "The sweep only removes `.part` files (`strings.HasSuffix(e.Name(), \".part\")`). A `.meta` file whose matching `.part` was deleted (for example, by `discardStaging` deleting the part file after the meta was written, but before the meta was also removed — or by a server crash between the two `os.Remove` calls in `discardStaging`) will survive every sweep indefinitely. The accumulation is likely small and harmless in practice, but it is a leak. Consider adding a second pass that removes `.meta` files older than the TTL whose matching `.part` is absent.",
"ai_prompt": "In servers/disk/transfer.go, the sweepTransfers function at around line 226 only deletes files whose name ends in '.part'. A corresponding '.meta' file (for the same transfer id, but with the '.meta' suffix instead of '.part') will never be removed by the sweep if its matching '.part' is already gone. Extend the sweep loop (or add a second pass) to also remove '.meta' files older than transferTTL when no corresponding '.part' file exists in the same directory."
},
{
"path": "servers/disk/transfer.go",
"line": 105,
"side": "RIGHT",
"body": "`openTransfer` does a Stat to learn `have`, then opens the file — no lock between those two steps. Two concurrent RPC calls with the same transfer id (say, a misbehaving client or two retries racing a stale connection) could both see `have == offset`, both pass the check, and then both append to the file, interleaving their writes. The server has no mutex protecting this operation. A simple per-id mutex (or a `sync.Map[string, *sync.Mutex]`) acquired around the Stat+Open pair would close the race.",
"ai_prompt": "In servers/disk/transfer.go around lines 82-110, the openTransfer function reads the current file size with os.Stat and then opens the file for appending. There is a TOCTOU race: two concurrent calls with the same transfer id can both pass the offset check and then both open and append to the file. Add a per-id mutex (for example, using a sync.Map to store *sync.Mutex keyed by transfer id) that is held for the entire Stat+check+OpenFile sequence. The Server struct should own the mutex map. Apply the same protection to the loadStaging/saveStaging pair in stageForClient for the same reason."
}
],
"posted_to_pr": true
}