testing: retry spurious errors from RemoveAll for temp directories

This works around what appears to be either a kernel bug or a Go
runtime or syscall bug affecting certain Windows versions
(possibly all pre-2016?).

The retry loop is a simplified version of the one used in
cmd/go/internal/robustio. We use the same 2-second arbitrary timeout
as was used in that package, since it seems to be reliable in practice
on the affected builders. (If it proves to be too short, we can
lengthen it, within reason, in a followup CL.)

Since this puts a higher-level workaround in place, we can also revert
the lower-level workaround added to a specific test in CL 345670.

This addresses the specific occurrences of the bug for users of
(*testing.T).TempDir, but does not fix the underlying bug for Go users
outside the "testing" package (which remains open as #25965).

Fixes #50051
Updates #48012
Updates #25965

Change-Id: I35be7125f32f05c8350787f5ca9a22974b8d0770
Reviewed-on: https://go-review.googlesource.com/c/go/+/371296
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Patrik Nyblom <pnyb@google.com>
Trust: Patrik Nyblom <pnyb@google.com>
Run-TryBot: Patrik Nyblom <pnyb@google.com>

diff --git a/src/runtime/syscall_windows_test.go b/src/runtime/syscall_windows_test.go
index 101e94107c5f5b50b86bb6520553165f90bfc8d9..dcd390ff9d4226cbdaaaa73e875ba79529d14212 100644 (file)
--- a/src/runtime/syscall_windows_test.go
+++ b/src/runtime/syscall_windows_test.go
@@ -770,6 +770,7 @@ func TestSyscallN(t *testing.T) {
        for arglen := 0; arglen <= runtime.MaxArgs; arglen++ {
                arglen := arglen
                t.Run(fmt.Sprintf("arg-%d", arglen), func(t *testing.T) {
+                       t.Parallel()
                        args := make([]string, arglen)
                        rets := make([]string, arglen+1)
                        params := make([]uintptr, arglen)

diff --git a/src/testing/testing.go b/src/testing/testing.go
index 7bd13a850c0b3281f84d27c9b84a605decb52395..a8c8122aa7c41cd98086f61ade66094ccaf63775 100644 (file)
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -1087,7 +1087,7 @@ func (c *common) TempDir() string {
                c.tempDir, c.tempDirErr = os.MkdirTemp("", pattern)
                if c.tempDirErr == nil {
                        c.Cleanup(func() {
-                               if err := os.RemoveAll(c.tempDir); err != nil {
+                               if err := removeAll(c.tempDir); err != nil {
                                        c.Errorf("TempDir RemoveAll cleanup: %v", err)
                                }
                        })
@@ -1106,6 +1106,36 @@ func (c *common) TempDir() string {
        return dir
 }
 
+// removeAll is like os.RemoveAll, but retries Windows "Access is denied."
+// errors up to an arbitrary timeout.
+//
+// Those errors have been known to occur spuriously on at least the
+// windows-amd64-2012 builder (https://go.dev/issue/50051), and can only occur
+// legitimately if the test leaves behind a temp file that either is still open
+// or the test otherwise lacks permission to delete. In the case of legitimate
+// failures, a failing test may take a bit longer to fail, but once the test is
+// fixed the extra latency will go away.
+func removeAll(path string) error {
+       const arbitraryTimeout = 2 * time.Second
+       var (
+               start     time.Time
+               nextSleep = 1 * time.Millisecond
+       )
+       for {
+               err := os.RemoveAll(path)
+               if !isWindowsAccessDenied(err) {
+                       return err
+               }
+               if start.IsZero() {
+                       start = time.Now()
+               } else if d := time.Since(start) + nextSleep; d >= arbitraryTimeout {
+                       return err
+               }
+               time.Sleep(nextSleep)
+               nextSleep += time.Duration(rand.Int63n(int64(nextSleep)))
+       }
+}
+
 // Setenv calls os.Setenv(key, value) and uses Cleanup to
 // restore the environment variable to its original value
 // after the test.

diff --git a/src/testing/testing_other.go b/src/testing/testing_other.go
new file mode 100644 (file)
index 0000000..29496d8
--- /dev/null
+++ b/src/testing/testing_other.go
@@ -0,0 +1,13 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build !windows
+
+package testing
+
+// isWindowsAccessDenied reports whether err is ERROR_ACCESS_DENIED,
+// which is defined only on Windows.
+func isWindowsAccessDenied(err error) bool {
+       return false
+}

diff --git a/src/testing/testing_windows.go b/src/testing/testing_windows.go
new file mode 100644 (file)
index 0000000..bc76cb8
--- /dev/null
+++ b/src/testing/testing_windows.go
@@ -0,0 +1,18 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build windows
+
+package testing
+
+import (
+       "errors"
+       "syscall"
+)
+
+// isWindowsAccessDenied reports whether err is ERROR_ACCESS_DENIED,
+// which is defined only on Windows.
+func isWindowsAccessDenied(err error) bool {
+       return errors.Is(err, syscall.ERROR_ACCESS_DENIED)
+}