[release-branch.go1.13] cmd/go: make module zip extraction more robust

Currently, we extract module zip files to temporary directories, then
atomically rename them into place. On Windows, this can fail with
ERROR_ACCESS_DENIED if another process (antivirus) has files open
before the rename. In CL 220978, we repeated the rename operation in a
loop over 500 ms, but this didn't solve the problem for everyone.

A better solution will extract module zip files to their permanent
locations in the cache and will keep a ".partial" marker file,
indicating when a module hasn't been fully extracted (CL 221157).
This approach is not safe if current versions of Go access the module
cache concurrently, since the module directory is detected with a
single os.Stat.

In the interim, this CL makes two changes:

1. Flaky file system operations are repeated over 2000 ms to reduce
the chance of this error occurring.
2. cmd/go will now check for .partial files created by future
versions. If a .partial file is found, it will lock the lock file,
then remove the .partial file and directory if needed.

After some time has passed and Go versions lacking this CL are no
longer supported, we can start extracting module zip files in place.

Updates #37802

Change-Id: I467ee11aa59a90b63cf0e3e761c4fec89d57d3b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/221820
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
(cherry picked from commit 093049b3709eda7537ece92a2991918cf53782d6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/223146

6 files changed, 141 insertions, 34 deletions

diff --git a/src/cmd/go/internal/modcmd/verify.go b/src/cmd/go/internal/modcmd/verify.go
index 81fc44dc97..dd1665619b 100644
--- a/src/cmd/go/internal/modcmd/verify.go
+++ b/src/cmd/go/internal/modcmd/verify.go
@@ -7,6 +7,7 @@ package modcmd
 import (
 	"bytes"
 	"cmd/go/internal/cfg"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -61,12 +62,10 @@ func verifyMod(mod module.Version) bool {
 		_, zipErr = os.Stat(zip)
 	}
 	dir, dirErr := modfetch.DownloadDir(mod)
-	if dirErr == nil {
-		_, dirErr = os.Stat(dir)
-	}
 	data, err := ioutil.ReadFile(zip + "hash")
 	if err != nil {
-		if zipErr != nil && os.IsNotExist(zipErr) && dirErr != nil && os.IsNotExist(dirErr) {
+		if zipErr != nil && errors.Is(zipErr, os.ErrNotExist) &&
+			dirErr != nil && errors.Is(dirErr, os.ErrNotExist) {
 			// Nothing downloaded yet. Nothing to verify.
 			return true
 		}
@@ -75,7 +74,7 @@ func verifyMod(mod module.Version) bool {
 	}
 	h := string(bytes.TrimSpace(data))
 
-	if zipErr != nil && os.IsNotExist(zipErr) {
+	if zipErr != nil && errors.Is(zipErr, os.ErrNotExist) {
 		// ok
 	} else {
 		hZ, err := dirhash.HashZip(zip, dirhash.DefaultHash)
@@ -87,7 +86,7 @@ func verifyMod(mod module.Version) bool {
 			ok = false
 		}
 	}
-	if dirErr != nil && os.IsNotExist(dirErr) {
+	if dirErr != nil && errors.Is(dirErr, os.ErrNotExist) {
 		// ok
 	} else {
 		hD, err := dirhash.HashDir(dir, mod.Path+"@"+mod.Version, dirhash.DefaultHash)
diff --git a/src/cmd/go/internal/modfetch/cache.go b/src/cmd/go/internal/modfetch/cache.go
index c0062809d1..446c8fec8b 100644
--- a/src/cmd/go/internal/modfetch/cache.go
+++ b/src/cmd/go/internal/modfetch/cache.go
@@ -7,6 +7,7 @@ package modfetch
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -57,8 +58,11 @@ func CachePath(m module.Version, suffix string) (string, error) {
 	return filepath.Join(dir, encVer+"."+suffix), nil
 }
 
-// DownloadDir returns the directory to which m should be downloaded.
-// Note that the directory may not yet exist.
+// DownloadDir returns the directory to which m should have been downloaded.
+// An error will be returned if the module path or version cannot be escaped.
+// An error satisfying errors.Is(err, os.ErrNotExist) will be returned
+// along with the directory if the directory does not exist or if the directory
+// is not completely populated.
 func DownloadDir(m module.Version) (string, error) {
 	if PkgMod == "" {
 		return "", fmt.Errorf("internal error: modfetch.PkgMod not set")
@@ -77,9 +81,39 @@ func DownloadDir(m module.Version) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return filepath.Join(PkgMod, enc+"@"+encVer), nil
+
+	dir := filepath.Join(PkgMod, enc+"@"+encVer)
+	if fi, err := os.Stat(dir); os.IsNotExist(err) {
+		return dir, err
+	} else if err != nil {
+		return dir, &DownloadDirPartialError{dir, err}
+	} else if !fi.IsDir() {
+		return dir, &DownloadDirPartialError{dir, errors.New("not a directory")}
+	}
+	partialPath, err := CachePath(m, "partial")
+	if err != nil {
+		return dir, err
+	}
+	if _, err := os.Stat(partialPath); err == nil {
+		return dir, &DownloadDirPartialError{dir, errors.New("not completely extracted")}
+	} else if !os.IsNotExist(err) {
+		return dir, err
+	}
+	return dir, nil
+}
+
+// DownloadDirPartialError is returned by DownloadDir if a module directory
+// exists but was not completely populated.
+//
+// DownloadDirPartialError is equivalent to os.ErrNotExist.
+type DownloadDirPartialError struct {
+	Dir string
+	Err error
 }
 
+func (e *DownloadDirPartialError) Error() string     { return fmt.Sprintf("%s: %v", e.Dir, e.Err) }
+func (e *DownloadDirPartialError) Is(err error) bool { return err == os.ErrNotExist }
+
 // lockVersion locks a file within the module cache that guards the downloading
 // and extraction of the zipfile for the given module version.
 func lockVersion(mod module.Version) (unlock func(), err error) {
diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go
index 51a56028c4..d5516dd8d9 100644
--- a/src/cmd/go/internal/modfetch/fetch.go
+++ b/src/cmd/go/internal/modfetch/fetch.go
@@ -7,6 +7,7 @@ package modfetch
 import (
 	"archive/zip"
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -22,6 +23,7 @@ import (
 	"cmd/go/internal/module"
 	"cmd/go/internal/par"
 	"cmd/go/internal/renameio"
+	"cmd/go/internal/robustio"
 )
 
 var downloadCache par.Cache
@@ -41,24 +43,27 @@ func Download(mod module.Version) (dir string, err error) {
 		err error
 	}
 	c := downloadCache.Do(mod, func() interface{} {
-		dir, err := DownloadDir(mod)
+		dir, err := download(mod)
 		if err != nil {
 			return cached{"", err}
 		}
-		if err := download(mod, dir); err != nil {
-			return cached{"", err}
-		}
 		checkMod(mod)
 		return cached{dir, nil}
 	}).(cached)
 	return c.dir, c.err
 }
 
-func download(mod module.Version, dir string) (err error) {
-	// If the directory exists, the module has already been extracted.
-	fi, err := os.Stat(dir)
-	if err == nil && fi.IsDir() {
-		return nil
+func download(mod module.Version) (dir string, err error) {
+	// If the directory exists, and no .partial file exists,
+	// the module has already been completely extracted.
+	// .partial files may be created when future versions of cmd/go
+	// extract module zip directories in place instead of extracting
+	// to a random temporary directory and renaming.
+	dir, err = DownloadDir(mod)
+	if err == nil {
+		return dir, nil
+	} else if dir == "" || !errors.Is(err, os.ErrNotExist) {
+		return "", err
 	}
 
 	// To avoid cluttering the cache with extraneous files,
@@ -66,7 +71,7 @@ func download(mod module.Version, dir string) (err error) {
 	// Invoke DownloadZip before locking the file.
 	zipfile, err := DownloadZip(mod)
 	if err != nil {
-		return err
+		return "", err
 	}
 
 	if cfg.CmdName != "mod download" {
@@ -75,17 +80,19 @@ func download(mod module.Version, dir string) (err error) {
 
 	unlock, err := lockVersion(mod)
 	if err != nil {
-		return err
+		return "", err
 	}
 	defer unlock()
 
 	// Check whether the directory was populated while we were waiting on the lock.
-	fi, err = os.Stat(dir)
-	if err == nil && fi.IsDir() {
-		return nil
+	_, dirErr := DownloadDir(mod)
+	if dirErr == nil {
+		return dir, nil
 	}
+	_, dirExists := dirErr.(*DownloadDirPartialError)
 
-	// Clean up any remaining temporary directories from previous runs.
+	// Clean up any remaining temporary directories from previous runs, as well
+	// as partially extracted diectories created by future versions of cmd/go.
 	// This is only safe to do because the lock file ensures that their writers
 	// are no longer active.
 	parentDir := filepath.Dir(dir)
@@ -95,6 +102,19 @@ func download(mod module.Version, dir string) (err error) {
 			RemoveAll(path) // best effort
 		}
 	}
+	if dirExists {
+		if err := RemoveAll(dir); err != nil {
+			return "", err
+		}
+	}
+
+	partialPath, err := CachePath(mod, "partial")
+	if err != nil {
+		return "", err
+	}
+	if err := os.Remove(partialPath); err != nil && !os.IsNotExist(err) {
+		return "", err
+	}
 
 	// Extract the zip file to a temporary directory, then rename it to the
 	// final path. That way, we can use the existence of the source directory to
@@ -102,11 +122,11 @@ func download(mod module.Version, dir string) (err error) {
 	// the entire directory (e.g. as an attempt to prune out file corruption)
 	// the module cache will still be left in a recoverable state.
 	if err := os.MkdirAll(parentDir, 0777); err != nil {
-		return err
+		return "", err
 	}
 	tmpDir, err := ioutil.TempDir(parentDir, tmpPrefix)
 	if err != nil {
-		return err
+		return "", err
 	}
 	defer func() {
 		if err != nil {
@@ -117,17 +137,17 @@ func download(mod module.Version, dir string) (err error) {
 	modpath := mod.Path + "@" + mod.Version
 	if err := Unzip(tmpDir, zipfile, modpath, 0); err != nil {
 		fmt.Fprintf(os.Stderr, "-> %s\n", err)
-		return err
+		return "", err
 	}
 
-	if err := os.Rename(tmpDir, dir); err != nil {
-		return err
+	if err := robustio.Rename(tmpDir, dir); err != nil {
+		return "", err
 	}
 
 	// Make dir read-only only *after* renaming it.
 	// os.Rename was observed to fail for read-only directories on macOS.
 	makeDirsReadOnly(dir)
-	return nil
+	return dir, nil
 }
 
 var downloadZipCache par.Cache
diff --git a/src/cmd/go/internal/modload/build.go b/src/cmd/go/internal/modload/build.go
index ff42516c80..a0c6b0bf74 100644
--- a/src/cmd/go/internal/modload/build.go
+++ b/src/cmd/go/internal/modload/build.go
@@ -143,9 +143,7 @@ func moduleInfo(m module.Version, fromBuildList bool) *modinfo.ModulePublic {
 			}
 			dir, err := modfetch.DownloadDir(mod)
 			if err == nil {
-				if info, err := os.Stat(dir); err == nil && info.IsDir() {
-					m.Dir = dir
-				}
+				m.Dir = dir
 			}
 		}
 	}
diff --git a/src/cmd/go/internal/robustio/robustio_windows.go b/src/cmd/go/internal/robustio/robustio_windows.go
index a3d94e566f..1eb8cfbccf 100644
--- a/src/cmd/go/internal/robustio/robustio_windows.go
+++ b/src/cmd/go/internal/robustio/robustio_windows.go
@@ -14,7 +14,7 @@ import (
 	"time"
 )
 
-const arbitraryTimeout = 500 * time.Millisecond
+const arbitraryTimeout = 2000 * time.Millisecond
 
 // retry retries ephemeral errors from f up to an arbitrary timeout
 // to work around spurious filesystem errors on Windows
diff --git a/src/cmd/go/testdata/script/mod_download_partial.txt b/src/cmd/go/testdata/script/mod_download_partial.txt
new file mode 100644
index 0000000000..cda358b99f
--- /dev/null
+++ b/src/cmd/go/testdata/script/mod_download_partial.txt
@@ -0,0 +1,56 @@
+# Download a module
+go mod download rsc.io/quote
+exists $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+
+# 'go mod verify' should fail if we delete a file.
+go mod verify
+chmod 0755 $GOPATH/pkg/mod/rsc.io/quote@v1.5.2
+rm $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+! go mod verify
+
+# Create a .partial file to simulate an failure extracting the zip file.
+cp empty $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.partial
+
+# 'go mod verify' should not fail, since the module hasn't been completely
+# ingested into the cache.
+go mod verify
+
+# 'go list' should not load packages from the directory.
+# NOTE: the message "directory $dir outside available modules" is reported
+# for directories not in the main module, active modules in the module cache,
+# or local replacements. In this case, the directory is in the right place,
+# but it's incomplete, so 'go list' acts as if it's not an active module.
+! go list $GOPATH/pkg/mod/rsc.io/quote@v1.5.2
+stderr 'outside available modules'
+
+# 'go list -m' should not print the directory.
+go list -m -f '{{.Dir}}' rsc.io/quote
+! stdout .
+
+# 'go mod download' should re-extract the module and remove the .partial file.
+go mod download rsc.io/quote
+! exists $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.partial
+exists $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+
+# 'go list' should succeed.
+go list $GOPATH/pkg/mod/rsc.io/quote@v1.5.2
+stdout '^rsc.io/quote$'
+
+# 'go list -m' should print the directory.
+go list -m -f '{{.Dir}}' rsc.io/quote
+stdout 'pkg[/\\]mod[/\\]rsc.io[/\\]quote@v1.5.2'
+
+# go mod verify should fail if we delete a file.
+go mod verify
+chmod 0755 $GOPATH/pkg/mod/rsc.io/quote@v1.5.2
+rm $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/go.mod
+! go mod verify
+
+-- go.mod --
+module m
+
+go 1.14
+
+require rsc.io/quote v1.5.2
+
+-- empty --