fix(backup): clean up .part files and bound skip tolerance (#555, #556)

[xe-nvdk]

Closes #555. Closes #556.

Two follow-ups split out of #322 / PR #554, now rebased on main with #554 merged.

#555 — orphaned .part files

LocalBackend.WriteReader writes to a deterministic <path>.part staging file and deliberately leaves it on failure so the file-replication puller can resume from the last committed byte (local.go:151-156).

Backup has no resume path — a retried backup starts over under a fresh backupID — so that staging file was unreferenced garbage: never read, never listed as a backup (no manifest.json, so ListBackups won't surface it), holding disk equal to the bytes transferred before the failure.

streamBackupFile now removes it on write failure. Best-effort by design: the write already failed, so the delete very likely fails too (unwritable volume, storage unreachable). A cleanup failure must not mask the original write error — there's a test for exactly that.

#556 — bounded skip tolerance

Skipping exists to tolerate one narrow race: a file removed by compaction or retention between the listing and the copy. That touches a handful of files at the tail of a run.

A large fraction of the backup failing to read is a different event — throttling, credential expiry, a storage outage — and returning a fraction of the data as a successful backup is how an operator discovers the gap at restore time instead of at backup time. #554 caught only the total-loss case; 4,000 of 10,000 files skipped still reported success.

Backups now fail above maxSkipRatio (10%) with source storage may be degraded. The skip count is still recorded on the failed run.

Two decisions worth naming:

• The ratio check subsumes the old all-skipped guard (100% exceeds any ratio), so total-loss behavior is unchanged. A test pins it explicitly rather than relying on that implication.
• maxSkipRatio is a constant, not a config key. No operator has asked to tune it, and per CLAUDE.md every new config key needs v.SetDefault plus testing at a non-default value — real work for a knob nobody sets. Easy to promote later if someone asks.

Configuration matrix

Both changes live in internal/backup, reachable only via the backup API. No new config keys, no new pointer dereferences, no startup wiring touched.

Configuration	Reaches new code?	Preconditions established?
OSS, backup API unused	No — Manager never constructed	n/a
OSS + backup, healthy storage	Partial — ratio check runs, skipped == 0 early-returns (backup.go:260)	backupStorage always LocalBackend (manager.go:53, sole assignment)
Backup, isolated file removed mid-run (compaction race)	Yes — skip counted, under ratio, backup succeeds	Delete returns nil for a missing path (local.go:476-479), so no spurious cleanup noise
Backup, >10% of sources unreadable (storage outage)	Yes — new failure path	len(files) > 0 guaranteed by the skipped > 0 precondition
Backup, write fails mid-transfer	Yes — cleanupPartialWrite	backupStorage non-nil per above; cleanup error swallowed at debug, original error preserved
Backup + S3/Azure dataStorage	Yes	Unchanged from #554 — size only ever reaches LocalBackend.WriteReader, which ignores it

Test plan

• TestStreamBackupFile_CleansUpPartFileOnWriteFailure (new) — verified it fails with the cleanup call removed: orphaned .part file left in backup storage: …/a.parquet.part
• TestStreamBackupFile_CleanupFailureDoesNotMaskWriteError (new) — cleanup failure preserves the original error and stays fatal (not reclassified as skippable)
• TestCopyDataFiles_SkipRatioExceeded (new) — verified it fails against fix(backup): stream files instead of buffering them in memory (#322) #554's all-skipped-only guard: expected failure when the skip ratio is exceeded, got nil (partial backup would report success)
• TestCopyDataFiles_AllFilesSkippedIsFatal — total loss still fails (now via the ratio path)
• TestCopyDataFiles_SkipsFailedFiles / _PartialSkipRecordsCount — rewritten to 21-file sets so one skip sits under the ratio; both assert SkippedFiles
• End-to-end through the real CreateBackup: 20 files → files=20 skipped=0, zero .part files anywhere under the backup root
• go build ./cmd/... ./internal/... clean
• go test ./internal/backup/... — 17/17 pass
• go test -race ./internal/backup/... clean
• go vet + gofmt -l clean

Note on the two rewritten tests: they previously used 2-file sets where a single skip is 50%, which now correctly trips the ratio. Enlarging them was the right fix — a 1-in-2 skip should fail under this policy.

🤖 Generated with Claude Code