backup: PR810 r5 — coderabbit Major (Close error on WriteChecksums)

Coderabbit r5 Major (checksums.go:82): WriteChecksums returned
out.Sync() but discarded the deferred out.Close() error. Same
shape as the r4 gemini medium on emitManifest, applied here too.
On NFS / some FUSE backends Close is where buffered-writeback
failures surface, so a swallowed Close means the CLI can declare
success on a truncated or unreadable CHECKSUMS file.

Fix: explicit Sync() then Close() with both errors propagated,
with the pre-existing defer'd Close swallowed only on the
error-return paths (the primary failure has already been
surfaced). Extracted writeAllChecksumLines so the parent stays
a thin open / write / sync / close skeleton with the durability
contract spelled out.

Caller audit (CLAUDE.md "semantic-change → grep all callers"):
- WriteChecksums has one caller
  (cmd/elastickv-snapshot-decode/main.go:253), which already
  surfaces the returned error verbatim via errors.Wrap. The
  semantic tightening (Close errors now surface) is honored
  by the existing wrapping; no caller change needed.

Also addresses coderabbit r5 nitpick: typed-sentinel assertion
in TestVerifyChecksums_DetectsTampering (was checking err !=
nil generically; now pins ErrChecksumMismatch so a future
change that turns the tamper signal into a different error
class surfaces as a test failure).

Self-review:
1. Data loss — fix prevents silent CHECKSUMS-truncation on
   NFS / FUSE.
2. Concurrency — none.
3. Performance — no new I/O; just surfaces an existing error.
4. Data consistency — durability contract now honest.
5. Test coverage — typed-sentinel assertion added; existing
   round-trip tests still pass.

Author: bootjp

internal/backup/checksums.go

@@ -72,14 +72,40 @@ func WriteChecksums(root string) error {
 	if err != nil {
 		return errors.WithStack(err)
 	}
-	defer func() { _ = out.Close() }()
+	if err := writeAllChecksumLines(out, entries); err != nil {
+		_ = out.Close()
+		return err
+	}
+	if err := out.Sync(); err != nil {
+		_ = out.Close()
+		return errors.WithStack(err)
+	}
+	// Surface Close errors instead of swallowing them. On NFS /
+	// some FUSE backends Close is the authoritative durability
+	// signal — writeback failures buffered after the Sync()
+	// fsync surface here, not at Write time. A swallowed Close
+	// would let the CLI declare success with a truncated or
+	// unreadable CHECKSUMS file. coderabbit Major on PR #810
+	// round 4 (same shape as the gemini r1 medium on
+	// emitManifest, applied here too).
+	if err := out.Close(); err != nil {
+		return errors.WithStack(err)
+	}
+	return nil
+}
+
+// writeAllChecksumLines emits the formatted CHECKSUMS rows. Split
+// out from WriteChecksums so the parent stays a thin
+// open-write-sync-close skeleton — its job is the durability
+// contract; the per-line escaping is delegated.
+func writeAllChecksumLines(w io.Writer, entries []checksumEntry) error {
 	for _, e := range entries {
 		line, prefix := formatChecksumLine(e.hex, e.relPath)
-		if _, err := fmt.Fprintf(out, "%s%s\n", prefix, line); err != nil {
+		if _, err := fmt.Fprintf(w, "%s%s\n", prefix, line); err != nil {
 			return errors.WithStack(err)
 		}
 	}
-	return errors.WithStack(out.Sync())
+	return nil
 }
 
 // formatChecksumLine builds one CHECKSUMS row using GNU coreutils

internal/backup/checksums_test.go

@@ -103,7 +103,11 @@ func TestVerifyChecksums_HappyPath(t *testing.T) {
 
 // TestVerifyChecksums_DetectsTampering pins the failure mode: a
 // post-checksum write to one of the listed files must surface as
-// a verification error.
+// ErrChecksumMismatch (not just "some error"). Pinning the
+// typed sentinel keeps the contract honest — a future change
+// that turns this into ErrChecksumsMalformedLine or
+// ErrChecksumsPathTraversal would otherwise pass this test
+// silently. CodeRabbit r5 nitpick on PR #810.
 func TestVerifyChecksums_DetectsTampering(t *testing.T) {
 	t.Parallel()
 	root := t.TempDir()
@@ -112,9 +116,13 @@ func TestVerifyChecksums_DetectsTampering(t *testing.T) {
 		t.Fatalf("WriteChecksums: %v", err)
 	}
 	mustWrite(t, filepath.Join(root, "a.txt"), []byte("tampered"))
-	if err := VerifyChecksums(root); err == nil {
+	err := VerifyChecksums(root)
+	if err == nil {
 		t.Fatalf("expected VerifyChecksums to detect tampering, got nil")
 	}
+	if !errors.Is(err, ErrChecksumMismatch) {
+		t.Fatalf("err = %v, want ErrChecksumMismatch", err)
+	}
 }
 
 // TestVerifyChecksums_RejectsTraversalPath is the regression for