fileblob: os.Rename does not work between different mount points

[impact-merlinzerbe]

Describe the bug

Since 2693ff1 fileblob creates temporary files in os.TempDir. If the mount point of this directory is different from the directory the application is running in, the call to os.Rename (https://github.com/google/go-cloud/blob/37e3cd7ce28e7b8ff717c6e8352854b13921b496/blob/fileblob/fileblob.go#L775C2-L775C2) fails with the error invalid cross-device link

To Reproduce

mkdir other-mount
sudo mount -t tmpfs tmpfs other-mount

then run

b1, _ := fileblob.OpenBucket("other-mount", nil)
panic(b1.WriteAll(context.Background(), "test", []byte("test"), nil))

Expected behavior

No error.

Version

go version go1.20.7 linux/amd64
gocloud.dev v0.33.0

[vangent]

Hmm, sorry about that.

I'm not sure it's possible to fix both this bug and the one that led to that PR (leaked temp files). One option is to use os.TempDir only when the bucket's base is on the same mount as os.TempDir, but I'm not sure how to determine that.

I guess I could try a os.Rename from os.TempDir to see if it works, and if it fails, stop using it.

[vangent]

I considered using a "test" os.Rename, but it seems like a bad idea to me; it may result a leaked file that has nothing to do with anything.

Probably the best plan is to add an Option to not use os.TempDir; you'll have to set that when creating a bucket on a mount point that's different than os.TempDir.

[impact-merlinzerbe]

Thanks for your quick reaction. Unfortunately, my project only uses go-cloud indirectly via another package so I cannot set the provided option. I think os.TempDir is the right default and dynamic detection is a poor choice as you have already described.
I just want to provide the workaround I currently use, in case someone else runs into this problem:
You can overwrite the directory os.TempDir uses by setting the environment variable TMPDIR, e.g.:

TMPDIR=/folder/on/same/mountpoint go run .

[vangent]

Thanks, will add that to the docstring for fileblob.

[theoilie]

I considered using a "test" os.Rename, but it seems like a bad idea to me; it may result a leaked file that has nothing to do with anything.

@vangent I'm wondering if you could do this approach by replacing the 2 fileblob calls to os.Rename() (in the Close() function of writer and writerWithSidecar) with a new CopyAndDelete() function that uses io.Copy() to safely copy a stream across filesystem mounts (and keeps the same permissions)? All the other cleanup logic is still there and no new files would be created (so nothing would leak) if it looks roughly like:

func CopyAndDelete(src, dst string) error {
	// Open source file for reading
	srcFile, err := os.Open(src)
	if err != nil {
		return err
	}
	defer srcFile.Close()

	// Get the source file's permissions
	srcInfo, err := srcFile.Stat()
	if err != nil {
		return err
	}

	// Create destination file
	dstFile, err := os.OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_EXCL, srcInfo.Mode())
	if err != nil {
		return err
	}
	defer dstFile.Close()

	// Copy the source file's content to the destination file
	_, err = io.Copy(dstFile, srcFile)
	if err != nil {
		return err
	}

	// Delete the source file
	err = os.Remove(src)
	if err != nil {
		return err
	}

	return nil
}

[vangent]

I considered that, but it can result in partial writes and leaks (e.g., if the application exits during the copy, or after the copy but before the Remove); that's why we use Rename.

I think what's there now is reasonable; it work fine in most situations, and if you are working on a mount you can either tell it that via Options, or use the TMPDIR environment variable to put the temp files wherever you want.

[theoilie]

Got it, yeah the leak for unexpected exit during copy would be an issue. Thanks for your responsiveness.

For anyone else who will need to use the no_tmp_dir in Options, my compromise is to clear these .tmp files on startup (assuming you have no application use case creating .tmp files that you want to persist):

filepath.WalkDir(uri, func(path string, d os.DirEntry, err error) error {
			if err != nil {
				return err
			}

			if !d.IsDir() && strings.HasSuffix(d.Name(), ".tmp") {
				err = os.Remove(path)
			}
			return err
		})

[ganigeorgiev]

@vangent minor-inconvenience: if there is no fixed release schedule, could you consider bumping the package version to v0.34.0 in order to use the NoTempDir option?

[vangent]

could you consider bumping the package version

Done