From b3a68b9457d94a776f325145c2e9ef4d3e2980ae Mon Sep 17 00:00:00 2001
From: Marek Siarkowicz <siarkowicz@google.com>
Date: Fri, 20 Dec 2024 10:54:25 +0100
Subject: [PATCH] Prevent leaking file descriptor during snapshotting and
 provide better logging of errors

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
---
 client/v3/snapshot/v3_snapshot.go | 37 ++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/client/v3/snapshot/v3_snapshot.go b/client/v3/snapshot/v3_snapshot.go
index 324e497cfd0..2aea4cc644d 100644
--- a/client/v3/snapshot/v3_snapshot.go
+++ b/client/v3/snapshot/v3_snapshot.go
@@ -17,6 +17,7 @@ package snapshot
 import (
 	"context"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -54,16 +55,31 @@ func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, d
 	if err != nil {
 		return "", err
 	}
-	defer cli.Close()
+	defer func() {
+		err := cli.Close()
+		if err != nil {
+			lg.Error("Failed to close client", zap.Error(err))
+		}
+	}()
 
 	partpath := dbPath + ".part"
-	defer os.RemoveAll(partpath)
+	defer func() {
+		err := os.RemoveAll(partpath)
+		if err != nil {
+			lg.Error("Failed to cleanup .part file", zap.Error(err))
+		}
+	}()
 
-	var f *os.File
-	f, err = os.OpenFile(partpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode)
+	f, err := os.OpenFile(partpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode)
 	if err != nil {
 		return "", fmt.Errorf("could not open %s (%w)", partpath, err)
 	}
+	defer func() {
+		err := f.Close()
+		if err != nil && !errors.Is(err, os.ErrClosed) {
+			lg.Error("Could not close file descriptor", zap.Error(err))
+		}
+	}()
 	lg.Info("created temporary db file", zap.String("path", partpath))
 
 	start := time.Now()
@@ -71,21 +87,26 @@ func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, d
 	if err != nil {
 		return "", err
 	}
-	defer resp.Snapshot.Close()
+	defer func() {
+		err := resp.Snapshot.Close()
+		if err != nil {
+			lg.Error("Could not close snapshot stream", zap.Error(err))
+		}
+	}()
 	lg.Info("fetching snapshot", zap.String("endpoint", cfg.Endpoints[0]))
 	var size int64
 	size, err = io.Copy(f, resp.Snapshot)
 	if err != nil {
-		return resp.Version, err
+		return resp.Version, fmt.Errorf("could not write snapshot: %w", err)
 	}
 	if !hasChecksum(size) {
 		return resp.Version, fmt.Errorf("sha256 checksum not found [bytes: %d]", size)
 	}
 	if err = fileutil.Fsync(f); err != nil {
-		return resp.Version, err
+		return resp.Version, fmt.Errorf("could not fsync snapshot: %w", err)
 	}
 	if err = f.Close(); err != nil {
-		return resp.Version, err
+		return resp.Version, fmt.Errorf("could not close file descriptor: %w", err)
 	}
 	lg.Info("fetched snapshot",
 		zap.String("endpoint", cfg.Endpoints[0]),