Repository: kopia/kopia
Pull request #4513 (2025-04-18 05:09:57)

refactor(general): leverage `os.CreateTemp`
Use `os.CreateTemp` when creating temporary files in `tempfile.createUnixFallback` function


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/4513?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Attention: Patch coverage is `0%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 76.37%. Comparing base [(`cb455c6`)](https://app.codecov.io/gh/kopia/kopia/commit/cb455c699a04e54cb47e25a676d7d65063ce883e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) to head [(`41bfdbc`)](https://app.codecov.io/gh/kopia/kopia/commit/41bfdbc3de36e189c24a0ef6a772bd07d6d81e2f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).
> Report is 474 commits behind head on master.

| [Files with missing lines](https://app.codecov.io/gh/kopia/kopia/pull/4513?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Patch % | Lines |
|---|---|---|
| [internal/tempfile/tempfile\_unix\_fallback.go](https://app.codecov.io/gh/kopia/kopia/pull/4513?src=pr&el=tree&filepath=internal%2Ftempfile%2Ftempfile_unix_fallback.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-aW50ZXJuYWwvdGVtcGZpbGUvdGVtcGZpbGVfdW5peF9mYWxsYmFjay5nbw==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/4513?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4513      +/-   ##
==========================================
+ Coverage   75.86%   76.37%   +0.50%     
==========================================
  Files         470      528      +58     
  Lines       37301    40049    +2748     
==========================================
+ Hits        28299    30587    +2288     
- Misses       7071     7450     +379     
- Partials     1931     2012      +81     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/4513?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).

<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
julio-lopez: @Rohit-BM18 FYI

Patch:
diff --git a/internal/bigmap/bigmap_internal.go b/internal/bigmap/bigmap_internal.go
index f80e2c43..8f924c18 100644
--- a/internal/bigmap/bigmap_internal.go
+++ b/internal/bigmap/bigmap_internal.go
@@ -344,7 +344,7 @@ func (m *internalMap) newMemoryMappedSegment(ctx context.Context) (mmap.MMap, er
 
 // +checklocks:m.mu
 func (m *internalMap) maybeCreateMappedFile(ctx context.Context) (*os.File, error) {
-	f, err := tempfile.Create("")
+	f, err := tempfile.CreateAutoDelete()
 	if err != nil {
 		return nil, errors.Wrap(err, "unable to create memory-mapped file")
 	}
diff --git a/internal/tempfile/tempfile.go b/internal/tempfile/tempfile.go
index 85d687cf..1bd82346 100644
--- a/internal/tempfile/tempfile.go
+++ b/internal/tempfile/tempfile.go
@@ -1,13 +1,3 @@
 // Package tempfile provides a cross-platform abstraction for creating private
 // read-write temporary files which are automatically deleted when closed.
 package tempfile
-
-import "os"
-
-func tempDirOr(dir string) string {
-	if dir != "" {
-		return dir
-	}
-
-	return os.TempDir()
-}
diff --git a/internal/tempfile/tempfile_linux.go b/internal/tempfile/tempfile_linux.go
index a0694f92..d0598039 100644
--- a/internal/tempfile/tempfile_linux.go
+++ b/internal/tempfile/tempfile_linux.go
@@ -8,9 +8,11 @@
 	"golang.org/x/sys/unix"
 )
 
-// Create creates a temporary file that will be automatically deleted on close.
-func Create(dir string) (*os.File, error) {
-	dir = tempDirOr(dir)
+const permissions = 0o600
+
+// CreateAutoDelete creates a temporary file that will be automatically deleted on close.
+func CreateAutoDelete() (*os.File, error) {
+	dir := os.TempDir()
 
 	// on reasonably modern Linux (3.11 and above) O_TMPFILE is supported,
 	// which creates invisible, unlinked file in a given directory.
@@ -20,7 +22,7 @@ func Create(dir string) (*os.File, error) {
 	}
 
 	if errors.Is(err, syscall.EISDIR) || errors.Is(err, syscall.EOPNOTSUPP) {
-		return createUnixFallback(dir)
+		return createUnixFallback()
 	}
 
 	return nil, &os.PathError{
diff --git a/internal/tempfile/tempfile_unix_fallback.go b/internal/tempfile/tempfile_unix_fallback.go
index 565e3a6c..f1ac3cb0 100644
--- a/internal/tempfile/tempfile_unix_fallback.go
+++ b/internal/tempfile/tempfile_unix_fallback.go
@@ -5,25 +5,19 @@
 
 import (
 	"os"
-	"path/filepath"
 
-	"github.com/google/uuid"
 	"github.com/pkg/errors"
 )
 
-const permissions = 0o600
-
 // createUnixFallback creates a temporary file that does not need to be removed on close.
-func createUnixFallback(dir string) (*os.File, error) {
-	fullPath := filepath.Join(tempDirOr(dir), uuid.NewString())
-
-	f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_EXCL|os.O_RDWR, permissions) //nolint:gosec
+func createUnixFallback() (*os.File, error) {
+	f, err := os.CreateTemp("", "kt-")
 	if err != nil {
 		return nil, err //nolint:wrapcheck
 	}
 
 	// immediately remove/unlink the file while we keep the handle open.
-	if derr := os.Remove(fullPath); derr != nil {
+	if derr := os.Remove(f.Name()); derr != nil {
 		f.Close() //nolint:errcheck
 		return nil, errors.Wrap(derr, "unable to unlink temporary file")
 	}
diff --git a/internal/tempfile/tempfile_unix_nonlinux.go b/internal/tempfile/tempfile_unix_nonlinux.go
index b58fff52..b0f28acc 100644
--- a/internal/tempfile/tempfile_unix_nonlinux.go
+++ b/internal/tempfile/tempfile_unix_nonlinux.go
@@ -7,7 +7,7 @@
 	"os"
 )
 
-// Create creates a temporary file that does not need to be removed on close.
-func Create(dir string) (*os.File, error) {
-	return createUnixFallback(dir)
+// CreateAutoDelete creates a temporary file that does not need to be explicitly removed on close.
+func CreateAutoDelete() (*os.File, error) {
+	return createUnixFallback()
 }
diff --git a/internal/tempfile/tempfile_windows.go b/internal/tempfile/tempfile_windows.go
index 8a6b00b5..6d5cc3db 100644
--- a/internal/tempfile/tempfile_windows.go
+++ b/internal/tempfile/tempfile_windows.go
@@ -9,9 +9,9 @@
 	"golang.org/x/sys/windows"
 )
 
-// Create creates a temporary file that will be automatically deleted on close.
-func Create(dir string) (*os.File, error) {
-	fullpath := filepath.Join(tempDirOr(dir), uuid.NewString())
+// CreateAutoDelete creates a temporary file that will be automatically deleted on close.
+func CreateAutoDelete() (*os.File, error) {
+	fullpath := filepath.Join(os.TempDir(), uuid.NewString())
 
 	fname, err := syscall.UTF16PtrFromString(fullpath)
 	if err != nil {


Test patch:
diff --git a/internal/tempfile/tempfile_linux_fallback_test.go b/internal/tempfile/tempfile_linux_fallback_test.go
new file mode 100644
index 00000000..dbc88e48
--- /dev/null
+++ b/internal/tempfile/tempfile_linux_fallback_test.go
@@ -0,0 +1,16 @@
+//go:build linux
+// +build linux
+
+package tempfile
+
+import (
+	"testing"
+)
+
+// Explicitly test the create fallback function.
+// This test only applies to Linux to explicitly test the fallback function.
+// In other unix platforms the fallback is the default implementation, so it
+// is already tested in the tests for the Create function.
+func TestCreateFallback(t *testing.T) {
+	VerifyTempfile(t, createUnixFallback)
+}
diff --git a/internal/tempfile/tempfile_test.go b/internal/tempfile/tempfile_test.go
index 8c2dd08f..5069c964 100644
--- a/internal/tempfile/tempfile_test.go
+++ b/internal/tempfile/tempfile_test.go
@@ -1,47 +1,11 @@
 package tempfile_test
 
 import (
-	"io"
-	"os"
 	"testing"
 
-	"github.com/stretchr/testify/require"
-
 	"github.com/kopia/kopia/internal/tempfile"
 )
 
 func TestTempFile(t *testing.T) {
-	td := t.TempDir()
-
-	f, err := tempfile.Create(td)
-	require.NoError(t, err)
-
-	n, err := f.WriteString("hello")
-	require.NoError(t, err)
-	require.Equal(t, 5, n)
-
-	off, err := f.Seek(1, io.SeekStart)
-	require.Equal(t, int64(1), off)
-	require.NoError(t, err)
-
-	buf := make([]byte, 4)
-	n2, err := f.Read(buf)
-	require.NoError(t, err)
-	require.Equal(t, 4, n2)
-	require.Equal(t, []byte("ello"), buf)
-
-	f.Close()
-
-	files, err := os.ReadDir(td)
-	require.NoError(t, err)
-	require.Empty(t, files)
-}
-
-func TestCreateSucceedsWhenDirIsNotSpecified(t *testing.T) {
-	f, err := tempfile.Create("")
-
-	require.NoError(t, err)
-
-	err = f.Close()
-	require.NoError(t, err)
+	tempfile.VerifyTempfile(t, tempfile.CreateAutoDelete)
 }
diff --git a/internal/tempfile/tempfile_verify_test.go b/internal/tempfile/tempfile_verify_test.go
new file mode 100644
index 00000000..5feeeac1
--- /dev/null
+++ b/internal/tempfile/tempfile_verify_test.go
@@ -0,0 +1,48 @@
+package tempfile
+
+import (
+	"io"
+	"os"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func VerifyTempfile(t *testing.T, create func() (*os.File, error)) {
+	t.Helper()
+
+	f, err := create()
+	require.NoError(t, err)
+
+	n, err := f.WriteString("hello")
+	require.NoError(t, err)
+	require.Equal(t, 5, n)
+
+	off, err := f.Seek(1, io.SeekStart)
+	require.Equal(t, int64(1), off)
+	require.NoError(t, err)
+
+	buf := make([]byte, 4)
+	n2, err := f.Read(buf)
+	require.NoError(t, err)
+	require.Equal(t, 4, n2)
+	require.Equal(t, []byte("ello"), buf)
+
+	f.Close()
+
+	if n := f.Name(); n != "" {
+		var perr *os.PathError
+
+		// there should be no directory entry for this file
+		_, err := os.Stat(n)
+
+		require.Error(t, err)
+		require.ErrorAs(t, err, &perr)
+		if runtime.GOOS == "windows" {
+			require.ErrorContains(t, err, "The system cannot find the file specified")
+		} else {
+			require.ErrorContains(t, err, "no such file or directory")
+		}
+	}
+}
diff --git a/tests/os_snapshot_test/os_snapshot_windows_test.go b/tests/os_snapshot_test/os_snapshot_windows_test.go
index 0127582d..01ff6b3a 100644
--- a/tests/os_snapshot_test/os_snapshot_windows_test.go
+++ b/tests/os_snapshot_test/os_snapshot_windows_test.go
@@ -2,13 +2,16 @@
 
 import (
 	"os"
+	"path/filepath"
+	"syscall"
 	"testing"
 
+	"github.com/google/uuid"
 	"github.com/mxk/go-vss"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
 
-	"github.com/kopia/kopia/internal/tempfile"
 	"github.com/kopia/kopia/internal/testutil"
 	"github.com/kopia/kopia/tests/clitestutil"
 	"github.com/kopia/kopia/tests/testenv"
@@ -21,21 +24,19 @@ func TestShadowCopy(t *testing.T) {
 	}
 
 	runner := testenv.NewExeRunnerWithBinary(t, kopiaExe)
-
 	e := testenv.NewCLITest(t, testenv.RepoFormatNotImportant, runner)
+
 	e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir)
+	e.RunAndExpectSuccess(t, "policy", "set", "--global", "--enable-volume-shadow-copy=when-available")
 
+	// create a file that cannot be accessed and then attempt to create a snapshot
 	root := testutil.TempDirectory(t)
-	f, err := tempfile.Create(root)
-	require.NoError(t, err)
-	_, err = f.WriteString("locked file\n")
+	f := createAutoDelete(t, root)
+	_, err := f.WriteString("locked file\n")
+
 	require.NoError(t, err)
 	require.NoError(t, f.Sync())
 
-	defer f.Close()
-
-	e.RunAndExpectSuccess(t, "policy", "set", "--global", "--enable-volume-shadow-copy=when-available")
-
 	_, err = vss.Get("{00000000-0000-0000-0000-000000000000}")
 
 	isAdmin := !errors.Is(err, os.ErrPermission)
@@ -43,7 +44,7 @@ func TestShadowCopy(t *testing.T) {
 		t.Log("Running as admin, expecting snapshot creation to succeed")
 		e.RunAndExpectSuccess(t, "snap", "create", root)
 	} else {
-		t.Log("Not running as admin, expecting snapshot creation to fail")
+		t.Log("Not running as admin, expecting snapshot creation to fail because it cannot access the file that is in use")
 		e.RunAndExpectFailure(t, "snap", "create", root)
 	}
 
@@ -54,11 +55,42 @@ func TestShadowCopy(t *testing.T) {
 
 	oid := sources[0].Snapshots[0].ObjectID
 	entries := clitestutil.ListDirectory(t, e, oid)
+	t.Log("sources[0].Snapshots[0].ObjectID entries:", entries)
 
 	if isAdmin {
+		require.NotEmpty(t, entries)
 		lines := e.RunAndExpectSuccess(t, "show", entries[0].ObjectID)
 		require.Equal(t, []string{"locked file"}, lines)
 	} else {
 		require.Empty(t, entries)
 	}
 }
+
+func createAutoDelete(t *testing.T, dir string) *os.File {
+	t.Helper()
+
+	fullpath := filepath.Join(dir, uuid.NewString())
+
+	fname, err := syscall.UTF16PtrFromString(fullpath)
+	require.NoError(t, err, "constructing file name UTF16Ptr")
+
+	// This call creates a file that's automatically deleted on close.
+	h, err := syscall.CreateFile(
+		fname,
+		windows.GENERIC_READ|windows.GENERIC_WRITE,
+		0,
+		nil,
+		syscall.OPEN_ALWAYS,
+		uint32(windows.FILE_FLAG_DELETE_ON_CLOSE),
+		0)
+
+	require.NoError(t, err, "creating file")
+
+	f := os.NewFile(uintptr(h), fullpath)
+
+	t.Cleanup(func() {
+		f.Close()
+	})
+
+	return f
+}