pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.5k stars 379 forks source link

Lstat failing on GCP #523

Closed inigomontoya1102 closed 1 year ago

inigomontoya1102 commented 1 year ago

Hi. I've got a program which connects to an SFTP server (hosted in Google Cloud), writes some files and then checks that the file was actually created. Something in the lines of:

outputPath := fmt.Sprintf("%s/%s", "myDir", "myFileName.txt")
// sftpClient is of type *sftp.Client
f, err := sftpClient.OpenFile(outputPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC)
if err != nil {
    return nil, fmt.Errorf("failed to create file %s. Error: %s", outputPath, err)
}
defer f.Close()

// write the file
if _, err := f.Write(data); err != nil {
    return nil, fmt.Errorf("file %s created by write failed: %s", outputPath, err)
}

// verify if the file exist
_, err := sftpClient.Lstat(outputPath)
if err != nil {
    return nil, fmt.Errorf("file %s was written by lstat failed: %s", outputPath, err)
}

...

the file is being written correctly, I can see it in the server in the specified location BUT when it reaches the Lstat command it is throwing an error:

"file /myDir/myFileName.txt was written by Lstat failed: file does not exist"

I'm using the package github.com/pkg/sftp.

Any idea what's going on?

drakkan commented 1 year ago

Hi,

try closing the file before Lstat. GCP uploads are atomic

inigomontoya1102 commented 1 year ago

@drakkan I'm deferring the file closing after opening it, you mean I shouldn't defer the close but actually closing it after writing to it

drakkan commented 1 year ago

yes, that way it should work, it also depends on the SFTP server are you using

inigomontoya1102 commented 1 year ago

did that yet i'm getting the same output

puellanivis commented 1 year ago

an SFTP server (hosted in Google Cloud)

Can you provide more details about what this SFTP server is, and how it was deployed? Is it an open source or off-the-shelf server/deployment, or are you building it manually? What is the SFTP server itself, and what version? That is, is it openssh sftp or some other server?

Are there any non-default flags or settings for it?

outputPath := fmt.Sprintf("%s/%s", "myDir", "myFileName.txt")

This is not building an absolute path—which our sftp implementation expects—and is an unsual way to build a path anyways. I would recommend using path.Join(…) in general, but this project also provides sftp.Join(…) (as a convenience wrapper around path.Join) intended specifically for the purpose of joining path elements together compliant with the SFTP standards.

puellanivis commented 1 year ago

Also, if you have changed your code to no longer defer your f.Close() be also aware that it can return an error, and that error could be that the file creation and file write failed. Some filesystems buffer all operations until file.Close() or file.Sync() is called, and does not perform any actual operations until then. This means unfortunately file.Close() can fail for critical reasons that mean that no file was actually created.

I know it’s super common to throw away the error from the Close() because of an assumption that it cannot ever fail in a meaningful way, but dropping that error on the floor can lead to really unexpected behavior in some cases.

inigomontoya1102 commented 1 year ago

that seemed to be the issue. For some reason my changes weren't pushed, but after checking and pushing once more I can see that you guys were right about the defer, the file needed to be closed before running lstat. It's weird because this was working fine on AWS (with defer) and stopped working on GCP. Thanks

drakkan commented 1 year ago

I'm surprised it works with AWS, anyway thanks for the confirmation

puellanivis commented 1 year ago

It’s great to hear that you found the fix. That it works fine on AWS but not on GCP can have any number of reasons. But it all kind of boils down to what I said about how some filesystems don’t actually commit anything until Sync() or Close() (the most notable is NFS). As so few filesystems actually do this, it’s a common thing for people to be unaware that opening a file with O_CREAT does not actually guarantee its existence until a sync() completes the whole write process.

If you search for “nfs files do not exist before synchronization” you’ll get a whole pile of results of people unexpectedly running into this same issue.

I’m going to go ahead and close the issue as resolved.