cmd/go/internal/cache: always check error from stat in markUsed

markUsed was not checking that the error from os.Stat was nil before
trying to access the FileInfo entry returned by it. Instead, always
check the error and return false if it's non-nil (usually because the
file does not exist). This can happen if an index entry exists in the
cache, but the output entry it points to does not. markUsed is called at
different points for the index entry and for the output entry, so it's
possible for the index entry to be marked used, and then for another go
process to trim the cache, deleting the output entry.  I'm not sure how
likely that is, or if this is what has been triggering the user observed
instances of #70600, but it's enough for a test case.

Fixes #70600

Change-Id: Ia6be14b4a56736d06488ccf93c3596fff8159f22
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/633037
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: matloob

src/cmd/go/internal/cache/cache.go

@@ -314,8 +314,8 @@ func GetMmap(c Cache, id ActionID) ([]byte, Entry, error) {
 // OutputFile returns the name of the cache file storing output with the given OutputID.
 func (c *DiskCache) OutputFile(out OutputID) string {
 	file := c.fileName(out, "d")
-	isExecutable := c.markUsed(file)
-	if isExecutable {
+	isDir := c.markUsed(file)
+	if isDir { // => cached executable
 		entries, err := os.ReadDir(file)
 		if err != nil {
 			return fmt.Sprintf("DO NOT USE - missing binary cache entry: %v", err)
@@ -357,12 +357,14 @@ const (
 // while still keeping the mtimes useful for cache trimming.
 //
 // markUsed reports whether the file is a directory (an executable cache entry).
-func (c *DiskCache) markUsed(file string) (isExecutable bool) {
+func (c *DiskCache) markUsed(file string) (isDir bool) {
 	info, err := os.Stat(file)
-	if err == nil && c.now().Sub(info.ModTime()) < mtimeInterval {
-		return info.IsDir()
+	if err != nil {
+		return false
+	}
+	if now := c.now(); now.Sub(info.ModTime()) >= mtimeInterval {
+		os.Chtimes(file, now, now)
 	}
-	os.Chtimes(file, c.now(), c.now())
 	return info.IsDir()
 }
 

src/cmd/go/testdata/script/list_issue_70600.txt

@@ -0,0 +1,43 @@
+# Test that the go command does not panic if it tries to read
+# a file from the cache that has an index entry, but is missing
+# an entry for the output. This test creates that situation by
+# running a go list (creating index and output cache entries for
+# the module index) and then removing just the output entries.
+
+[short] skip 'runs go build'
+
+go build -o roe$GOEXE ./remove_output_entries.go
+
+# populate new cache
+env GOCACHE=$WORK/newcache
+go list runtime
+
+# remove output entries and check the panic doesn't happen
+exec ./roe$GOEXE $WORK/newcache
+go list runtime
+
+-- remove_output_entries.go --
+package main
+
+import (
+	"io/fs"
+	"log"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+func main() {
+	cachedir := os.Args[1]
+	err := filepath.WalkDir(cachedir, func(path string, d fs.DirEntry, err error) error {
+		if strings.HasSuffix(path, "-d") { // output entries end in "-d"
+			if err := os.RemoveAll(path); err != nil {
+				return err
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		log.Fatal(err)
+	}
+}