From 0138923e6af6189362e9a742fb609a8ac96b8890 Mon Sep 17 00:00:00 2001 From: Aaron Son Date: Fri, 16 Jan 2026 15:13:11 -0800 Subject: [PATCH 1/3] go/cmd/dolt: Make movable temp file configuration lazy. Previously, early in the process life-cycle after dolt was run, dolt would immediately check if a file created in `os.TempDir()` was able to be `os.Rename`d into a subdirectory of the dolt process's data directory, by default `$PWD/.dolt`. If the rename failed, it would configure Dolt to use `.dolt/temptf` as the movable temp file directory instead. This meant that for many `dolt` invocations, Dolt would do some local filesystem writes, including potentially `Mkdir(.dolt)` before it it was fully loaded. With the advent of things like dolt accessing the running server through sql-server.info and `dolt --host ... sql` this behavior was not ideal. It creates races with concurrently run `dolt` processes that try to use the `.dolt` directory, and it creates odd behavior when running something like `dolt sql` in a non-Dolt directory but on a filesystem mount where this rename from `os.TempDir()` does not work. This PR changes the check and the configuration of a potential `.dolt/temptf` directory to be delayed until the first temporary movable file is actually requested by Dolt. At that point we know that we have a need for a renamable temp file and it is OK to go ahead and create `.dolt/temptf` if we need it. This PR changes the error handling around some edge cases like permissions errors on the calling process creating `temptf` within `.dolt`. In particular, some errors which were previously early and fatal are now delayed until use site and may end up being persistent but non-fatal to the process, depending on the operation. --- go/cmd/dolt/system_checks.go | 113 ++++++++++++++------------ go/store/util/tempfiles/temp_files.go | 42 ++++++++++ 2 files changed, 103 insertions(+), 52 deletions(-) diff --git a/go/cmd/dolt/system_checks.go b/go/cmd/dolt/system_checks.go index ff974fc8f8..d3ef53dfce 100644 --- a/go/cmd/dolt/system_checks.go +++ b/go/cmd/dolt/system_checks.go @@ -36,69 +36,78 @@ func reconfigIfTempFileMoveFails(dataDir filesys.Filesys) error { return err } - dotDoltCreated := false - tmpDirCreated := false + // Configure MovableTempFileProvider so that it lazily checks if os.TempDir() can be moved from to the data directory + // or not. If it cannot be, this will configure .dolt/temptf as the movable temp file directory. + // + // The intent of this being lazy is that we do not want to mess with the local filesystem unless we are asked to, + // but the concrete way we check for moveability is to create a temp file and move it into a known subdirectory + // of the .dolt subdirectory. We shouldn't create any of those things unless we need to because we are actually + // doing filesystem writes. + origprovider := tempfiles.MovableTempFileProvider + tempfiles.MovableTempFileProvider = tempfiles.NewLazyTempFileProvider(func() (tempfiles.TempFileProvider, error) { + dotDoltCreated := false + tmpDirCreated := false - doltDir := filepath.Join(absP, dbfactory.DoltDir) - stat, err := os.Stat(doltDir) - if err != nil { - err := os.MkdirAll(doltDir, os.ModePerm) + doltDir := filepath.Join(absP, dbfactory.DoltDir) + stat, err := os.Stat(doltDir) if err != nil { - return fmt.Errorf("failed to create dolt dir '%s': %s", doltDir, err.Error()) + err := os.MkdirAll(doltDir, os.ModePerm) + if err != nil { + return nil, fmt.Errorf("failed to create dolt dir '%s': %s", doltDir, err.Error()) + } + + dotDoltCreated = true } - dotDoltCreated = true - } - - doltTmpDir := filepath.Join(doltDir, env.TmpDirName) - stat, err = os.Stat(doltTmpDir) - if err != nil { - err := os.MkdirAll(doltTmpDir, os.ModePerm) + doltTmpDir := filepath.Join(doltDir, env.TmpDirName) + stat, err = os.Stat(doltTmpDir) if err != nil { - return fmt.Errorf("failed to create temp dir '%s': %s", doltTmpDir, err.Error()) - } - tmpDirCreated = true + err := os.MkdirAll(doltTmpDir, os.ModePerm) + if err != nil { + return nil, fmt.Errorf("failed to create temp dir '%s': %s", doltTmpDir, err.Error()) + } + tmpDirCreated = true - } else if !stat.IsDir() { - return fmt.Errorf("attempting to use '%s' as a temp directory, but there exists a file with that name", doltTmpDir) - } - - tmpF, err := os.CreateTemp("", "") - if err != nil { - return err - } - - name := tmpF.Name() - err = tmpF.Close() - if err != nil { - return err - } - - movedName := filepath.Join(doltTmpDir, "testfile") - - if os.Getenv("DOLT_FORCE_LOCAL_TEMP_FILES") == "" { - err = file.Rename(name, movedName) - } else { - err = errors.New("treating rename as failed because DOLT_FORCE_LOCAL_TEMP_FILES is set") - } - if err == nil { - // If rename was successful, then the tmp dir is fine, so no need to change it. Clean up the things we created. - _ = file.Remove(movedName) - - if tmpDirCreated { - _ = file.Remove(doltTmpDir) + } else if !stat.IsDir() { + return nil, fmt.Errorf("attempting to use '%s' as a temp directory, but there exists a file with that name", doltTmpDir) } - if dotDoltCreated { - _ = file.Remove(doltDir) + tmpF, err := os.CreateTemp("", "") + if err != nil { + return nil, err } - return nil - } - _ = file.Remove(name) + name := tmpF.Name() + err = tmpF.Close() + if err != nil { + return nil, err + } - // Rename failed. So we force the tmp dir to be the data dir. - tempfiles.MovableTempFileProvider = tempfiles.NewTempFileProviderAt(doltTmpDir) + movedName := filepath.Join(doltTmpDir, "testfile") + + if os.Getenv("DOLT_FORCE_LOCAL_TEMP_FILES") == "" { + err = file.Rename(name, movedName) + } else { + err = errors.New("treating rename as failed because DOLT_FORCE_LOCAL_TEMP_FILES is set") + } + if err == nil { + // If rename was successful, then the tmp dir is fine, so no need to change it. Clean up the things we created. + _ = file.Remove(movedName) + + if tmpDirCreated { + _ = file.Remove(doltTmpDir) + } + + if dotDoltCreated { + _ = file.Remove(doltDir) + } + + return origprovider, nil + } + _ = file.Remove(name) + // Rename failed. So we force the tmp dir to be the data dir. + return tempfiles.NewTempFileProviderAt(doltTmpDir), nil + }) return nil } diff --git a/go/store/util/tempfiles/temp_files.go b/go/store/util/tempfiles/temp_files.go index fd046a9e12..e187f662dd 100644 --- a/go/store/util/tempfiles/temp_files.go +++ b/go/store/util/tempfiles/temp_files.go @@ -80,6 +80,48 @@ func (tfp *TempFileProviderAt) Clean() { } } +type LazyTempFileProvider struct { + once sync.Once + loader func() (TempFileProvider, error) + provider TempFileProvider + perr error +} + +func NewLazyTempFileProvider(loader func() (TempFileProvider, error)) *LazyTempFileProvider { + return &LazyTempFileProvider{ + loader: loader, + } +} + +func (p *LazyTempFileProvider) loadit() { + p.once.Do(func() { + p.provider, p.perr = p.loader() + }) +} + +func (p *LazyTempFileProvider) Clean() { + // Don't load if we haven't already been loaded. + if p.provider != nil { + p.provider.Clean() + } +} + +func (p *LazyTempFileProvider) GetTempDir() string { + p.loadit() + if p.perr != nil { + return os.TempDir() + } + return p.provider.GetTempDir() +} + +func (p *LazyTempFileProvider) NewFile(dir, pattern string) (*os.File, error) { + p.loadit() + if p.perr != nil { + return nil, p.perr + } + return p.provider.NewFile(dir, pattern) +} + // MovableTemFile is an object that implements TempFileProvider that is used by the nbs to create temp files that // ultimately will be renamed. It is important to use this instance rather than using os.TempDir, or os.CreateTemp // directly as those may have errors executing a rename against if the volume the default temporary directory lives on From 5362a4ca254bcc4d00e596642ac5aeeb9df80467 Mon Sep 17 00:00:00 2001 From: Aaron Son Date: Fri, 16 Jan 2026 15:42:10 -0800 Subject: [PATCH 2/3] go/store/utils/tempfiles: PR feedback. Comment for LazyTempFileProvider. --- go/store/util/tempfiles/temp_files.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/go/store/util/tempfiles/temp_files.go b/go/store/util/tempfiles/temp_files.go index e187f662dd..8a63cf75c7 100644 --- a/go/store/util/tempfiles/temp_files.go +++ b/go/store/util/tempfiles/temp_files.go @@ -80,6 +80,12 @@ func (tfp *TempFileProviderAt) Clean() { } } +// LazyTempFileProvider will load the TempFileProvider from |loader| +// on first access and then return temp files based on that result +// going forward. This is configured for the dolt process's data +// directory to get our process-wide MovableTempFileProvider early in +// the Dolt process's life cycle, but the required capabilities are +// not checked for until first use. type LazyTempFileProvider struct { once sync.Once loader func() (TempFileProvider, error) From 0d0176025d3247ce2f8d068b8c6f7c0737d4960b Mon Sep 17 00:00:00 2001 From: Aaron Son Date: Fri, 16 Jan 2026 15:42:32 -0800 Subject: [PATCH 3/3] Update go/store/util/tempfiles/temp_files.go Co-authored-by: angelamayxie --- go/store/util/tempfiles/temp_files.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/store/util/tempfiles/temp_files.go b/go/store/util/tempfiles/temp_files.go index 8a63cf75c7..040d06fc58 100644 --- a/go/store/util/tempfiles/temp_files.go +++ b/go/store/util/tempfiles/temp_files.go @@ -128,7 +128,7 @@ func (p *LazyTempFileProvider) NewFile(dir, pattern string) (*os.File, error) { return p.provider.NewFile(dir, pattern) } -// MovableTemFile is an object that implements TempFileProvider that is used by the nbs to create temp files that +// MovableTempFile is an object that implements TempFileProvider that is used by the nbs to create temp files that // ultimately will be renamed. It is important to use this instance rather than using os.TempDir, or os.CreateTemp // directly as those may have errors executing a rename against if the volume the default temporary directory lives on // is different than the volume of the destination of the rename.