Basekick-Labs / Basekick-Labs/arc
backup/restore: the source→temp copy lost its zero-copy fast path to the tracking writer; get it back without losing failure attribution
- Dominant language
- Go
- Stars
- 677
- Forks
- 53
- Avg merge
- 9h 14m
- Merged PRs (30d)
- 164
Description
Follow-up to #778 (restore) and #784 (backup). Both wrap the per-file temp file in `trackingWriter` so a `ReadTo` failure can be attributed to the destination (temp filesystem full) rather than the source. The wrapper is deliberate and documented, but it has a cost: `io.Copy` only takes the `copy_file_range` path on Linux when the destination is an `*os.File`, so the source→temp hop now runs through a 32 KiB userspace loop. Restore is rare and disk-bound; backup runs on a schedule over the whole store, so on Linux with local data storage this is a measurable CPU cost per backup.
## Options
1. **Implement `io.ReaderFrom` on `trackingWriter`** delegating to the underlying `*os.File`'s `ReadFrom`, which restores `copy_file_range` when the source is also an `*os.File` (the local backend). The catch: a kernel copy reports one error for either side, so attribution has to come from somewhere else.
2. **Attribute on failure by probing the destination:** after a failed `ReadTo`, attempt a small write to the temp file. If the probe fails (ENOSPC, EDQUOT, EBADF, EIO on the temp volume) the destination is at fault; otherwise treat it as a source read. Heuristic, but it only runs on the failure path, and a transient condition that clears between failure and probe degrades to today's pre-#778 behaviour, not worse.
3. Keep the wrapper only for backends whose `ReadTo` is not a local file copy (S3/Azure stream into the temp file, where `copy_file_range` never applied anyway) and use the bare `*os.File` for `LocalBackend`. Simplest, but it means two code paths.
Measure before choosing: a 1 GiB file, `io.Copy(dst *os.File, src *os.File)` vs `io.Copy(&trackingWriter{dst}, src)` on Linux (macOS has no zero-copy path, so the difference cannot be seen there). The existing seam tests (`TestCopyDataFiles_TempWriteFailureIsFatalNotSkipped`, `TestRestore_TempWriteFailureIsFatalNotSkipped`) are the contract any option must keep green.
Contributor guide
Research direction
Start by benchmarking a 1 GiB local-file copy with io.Copy to an *os.File versus through trackingWriter on Linux, as described in the issue. Then inspect trackingWriter, LocalBackend, and the existing seam tests TestCopyDataFiles_TempWriteFailureIsFatalNotSkipped and TestRestore_TempWriteFailureIsFatalNotSkipped. Done means the chosen approach restores the measurable fast path while keeping both failure-attribution tests green.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, linux
- Domain
- backend, databases, performance
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100