Feature/freshen up windows workers

cmd/bb_runner/main.go

@@ -31,7 +31,7 @@ func main() {
 		}
 		var configuration bb_runner.ApplicationConfiguration
 		if err := util.UnmarshalConfigurationFromFile(os.Args[1], &configuration); err != nil {
-			return util.StatusWrapf(err, "Failed to read configuration from %s", os.Args[1])
+			return util.StatusWrapf(err, "Failed to read configuration from %#v", os.Args[1])
 		}
 		lifecycleState, grpcClientFactory, err := global.ApplyConfiguration(configuration.Global)
 		if err != nil {
@@ -40,7 +40,7 @@ func main() {
 
 		buildDirectoryPath, scopeWalker := path.EmptyBuilder.Join(path.NewAbsoluteScopeWalker(path.VoidComponentWalker))
 		if err := path.Resolve(path.NewLocalParser(configuration.BuildDirectoryPath), scopeWalker); err != nil {
-			return util.StatusWrap(err, "Failed to resolve build directory")
+			return util.StatusWrapf(err, "Failed to resolve build directory %#v", configuration.BuildDirectoryPath)
 		}
 		buildDirectory := re_filesystem.NewLazyDirectory(
 			func() (filesystem.DirectoryCloser, error) {

cmd/bb_worker/main.go

@@ -260,10 +260,10 @@ func main() {
 				// used. This increases cache hit rate.
 				cacheDirectory, err := filesystem.NewLocalDirectory(path.NewLocalParser(nativeConfiguration.CacheDirectoryPath))
 				if err != nil {
-					return util.StatusWrap(err, "Failed to open cache directory")
+					return util.StatusWrapf(err, "Failed to open cache directory %#v", nativeConfiguration.CacheDirectoryPath)
 				}
 				if err := cacheDirectory.RemoveAllChildren(); err != nil {
-					return util.StatusWrap(err, "Failed to clear cache directory")
+					return util.StatusWrapf(err, "Failed to clear cache directory %#v", nativeConfiguration.CacheDirectoryPath)
 				}
 				evictionSet, err := eviction.NewSetFromConfiguration[string](nativeConfiguration.CacheReplacementPolicy)
 				if err != nil {

pkg/builder/build_client.go

@@ -176,7 +176,7 @@ func (bc *BuildClient) touchSchedulerMayThinkExecuting() {
 	bc.schedulerMayThinkExecutingUntil = &until
 }
 
-// Run a iteration of the Remote Worker client, by performing a single
+// Run an iteration of the Remote Worker client, by performing a single
 // synchronization against the scheduler.
 func (bc *BuildClient) Run(ctx context.Context) (bool, error) {
 	// Allow the worker to terminate if the scheduler doesn't think

pkg/builder/clean_build_directory_creator.go

@@ -15,7 +15,7 @@ type cleanBuildDirectoryCreator struct {
 }
 
 // NewCleanBuildDirectoryCreator is an adapter for BuildDirectoryCreator
-// that upon acquistion and release calls into a Cleaner. This Cleaner
+// that upon acquisition and release calls into a Cleaner. This Cleaner
 // may, for example, be set up to empty out the build directory. This
 // guarantees that build actions aren't able to see data left behind by
 // ones that ran previously.

pkg/builder/local_build_executor.go

@@ -160,7 +160,14 @@ func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesyste
 			util.StatusWrap(err, "Failed to acquire build environment"))
 		return response
 	}
-	defer buildDirectory.Close()
+	defer func() {
+		err := buildDirectory.Close()
+		if err != nil {
+			attachErrorToExecuteResponse(
+				response,
+				util.StatusWrap(err, "Failed to close build directory"))
+		}
+	}()
 
 	// Install hooks on build directory to capture file creation and
 	// I/O error events.

pkg/runner/local_runner.go

@@ -3,7 +3,12 @@ package runner
 import (
 	"context"
 	"errors"
+	"math"
+	"os"
 	"os/exec"
+	"path/filepath"
+	"strings"
+	"syscall"
 
 	"github.com/buildbarn/bb-remote-execution/pkg/proto/runner"
 	"github.com/buildbarn/bb-storage/pkg/filesystem"
@@ -88,6 +93,37 @@ func (r *localRunner) openLog(logPath string) (filesystem.FileAppender, error) {
 // on whether the action needs to be run in a chroot() or not.
 type CommandCreator func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error)
 
+// NewPlainCommandCreator returns a CommandCreator for cases where we don't
+// need to chroot into the input root directory.
+func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator {
+	return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) {
+		workingDirectory, scopeWalker := inputRootDirectory.Join(path.VoidScopeWalker)
+		if err := path.Resolve(workingDirectoryParser, scopeWalker); err != nil {
+			return nil, util.StatusWrap(err, "Failed to resolve working directory")
+		}
+		workingDirectoryStr, err := path.GetLocalString(workingDirectory)
+		if err != nil {
+			return nil, util.StatusWrap(err, "Failed to create local representation of working directory")
+		}
+		executablePath, err := lookupExecutable(workingDirectory, pathVariable, arguments[0])
+		if err != nil {
+			return nil, err
+		}
+
+		// exec.CommandContext() has some smartness to call
+		// exec.LookPath() under the hood, which we don't want.
+		// Call it with a placeholder path, followed by setting
+		// cmd.Path and cmd.Args manually. This ensures that our
+		// own values remain respected.
+		cmd := exec.CommandContext(ctx, "/nonexistent")
+		cmd.Args = arguments
+		cmd.Dir = workingDirectoryStr
+		cmd.Path = executablePath
+		cmd.SysProcAttr = sysProcAttr
+		return cmd, nil
+	}
+}
+
 // NewLocalRunner returns a Runner capable of running commands on the
 // local system directly.
 func NewLocalRunner(buildDirectory filesystem.Directory, buildDirectoryPath *path.Builder, commandCreator CommandCreator, setTmpdirEnvironmentVariable bool) runner.RunnerServer {
@@ -201,3 +237,63 @@ func (r *localRunner) CheckReadiness(ctx context.Context, request *runner.CheckR
 
 	return &emptypb.Empty{}, nil
 }
+
+// getExecutablePath returns the path of an executable within a given
+// search path that is part of the PATH environment variable.
+func getExecutablePath(baseDirectory *path.Builder, searchPathStr, argv0 string) (string, error) {
+	searchPath, scopeWalker := baseDirectory.Join(path.VoidScopeWalker)
+	if err := path.Resolve(path.NewLocalParser(searchPathStr), scopeWalker); err != nil {
+		return "", err
+	}
+
+	executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker)
+	if err := path.Resolve(path.NewLocalParser(argv0), scopeWalker); err != nil {
+		return "", err
+	}
+	return path.GetLocalString(executablePath)
+}
+
+// lookupExecutable returns the path of an executable, taking the PATH
+// environment variable into account.
+func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) {
+	if strings.ContainsFunc(argv0, func(r rune) bool {
+		return r <= math.MaxUint8 && os.IsPathSeparator(uint8(r))
+	}) {
+		// No PATH processing needs to be performed.
+		return argv0, nil
+	}
+
+	// Executable path does not contain any slashes. Perform PATH
+	// lookups.
+	//
+	// We cannot use exec.LookPath() directly, as that function
+	// disregards the working directory of the action. It also uses
+	// the PATH environment variable of the current process, as
+	// opposed to respecting the value that is provided as part of
+	// the action. Do call into this function to validate the
+	// existence of the executable.
+	for _, searchPathStr := range filepath.SplitList(pathVariable) {
+		executablePathAbs, err := getExecutablePath(workingDirectory, searchPathStr, argv0)
+		if err != nil {
+			return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
+		}
+		if _, err := exec.LookPath(executablePathAbs); err == nil {
+			// Regular compiled executables will receive the
+			// argv[0] that we provide, but scripts starting
+			// with '#!' will receive the literal executable
+			// path.
+			//
+			// Most shells seem to guarantee that if argv[0]
+			// is relative, the executable path is relative
+			// as well. Prevent these scripts from breaking
+			// by recomputing the executable path once more,
+			// but relative.
+			executablePathRel, err := getExecutablePath(&path.EmptyBuilder, searchPathStr, argv0)
+			if err != nil {
+				return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
+			}
+			return executablePathRel, nil
+		}
+	}
+	return "", status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable)
+}

pkg/runner/local_runner_unix.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 	"syscall"
 
@@ -16,100 +15,9 @@ import (
 	"github.com/buildbarn/bb-storage/pkg/util"
 
 	"golang.org/x/sys/unix"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/durationpb"
 )
 
-// getExecutablePath returns the path of an executable within a given
-// search path that is part of the PATH environment variable.
-func getExecutablePath(baseDirectory *path.Builder, searchPathStr, argv0 string) (string, error) {
-	searchPath, scopeWalker := baseDirectory.Join(path.VoidScopeWalker)
-	if err := path.Resolve(path.NewLocalParser(searchPathStr), scopeWalker); err != nil {
-		return "", err
-	}
-
-	executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker)
-	if err := path.Resolve(path.NewLocalParser(argv0), scopeWalker); err != nil {
-		return "", err
-	}
-	return path.GetLocalString(executablePath)
-}
-
-// lookupExecutable returns the path of an executable, taking the PATH
-// environment variable into account.
-func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) {
-	if strings.ContainsRune(argv0, os.PathSeparator) {
-		// No PATH processing needs to be performed.
-		return argv0, nil
-	}
-
-	// Executable path does not contain any slashes. Perform PATH
-	// lookups.
-	//
-	// We cannot use exec.LookPath() directly, as that function
-	// disregards the working directory of the action. It also uses
-	// the PATH environment variable of the current process, as
-	// opposed to respecting the value that is provided as part of
-	// the action. Do call into this function to validate the
-	// existence of the executable.
-	for _, searchPathStr := range filepath.SplitList(pathVariable) {
-		executablePathAbs, err := getExecutablePath(workingDirectory, searchPathStr, argv0)
-		if err != nil {
-			return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
-		}
-		if _, err := exec.LookPath(executablePathAbs); err == nil {
-			// Regular compiled executables will receive the
-			// argv[0] that we provide, but scripts starting
-			// with '#!' will receive the literal executable
-			// path.
-			//
-			// Most shells seem to guarantee that if argv[0]
-			// is relative, the executable path is relative
-			// as well. Prevent these scripts from breaking
-			// by recomputing the executable path once more,
-			// but relative.
-			executablePathRel, err := getExecutablePath(&path.EmptyBuilder, searchPathStr, argv0)
-			if err != nil {
-				return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
-			}
-			return executablePathRel, nil
-		}
-	}
-	return "", status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable)
-}
-
-// NewPlainCommandCreator returns a CommandCreator for cases where we don't
-// need to chroot into the input root directory.
-func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator {
-	return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) {
-		workingDirectory, scopeWalker := inputRootDirectory.Join(path.VoidScopeWalker)
-		if err := path.Resolve(workingDirectoryParser, scopeWalker); err != nil {
-			return nil, util.StatusWrap(err, "Failed to resolve working directory")
-		}
-		workingDirectoryStr, err := path.GetLocalString(workingDirectory)
-		if err != nil {
-			return nil, util.StatusWrap(err, "Failed to create local representation of working directory")
-		}
-		executablePath, err := lookupExecutable(workingDirectory, pathVariable, arguments[0])
-		if err != nil {
-			return nil, err
-		}
-
-		// exec.CommandContext() has some smartness to call
-		// exec.LookPath() under the hood, which we don't want.
-		// Call it with a placeholder path, followed by setting
-		// cmd.Path and cmd.Args manually. This ensures that our
-		// own values remain respected.
-		cmd := exec.CommandContext(ctx, "/nonexistent")
-		cmd.Args = arguments
-		cmd.Dir = workingDirectoryStr
-		cmd.Path = executablePath
-		cmd.SysProcAttr = sysProcAttr
-		return cmd, nil
-	}
-}
-
 // NewChrootedCommandCreator returns a CommandCreator for cases where we
 // need to chroot into the input root directory.
 func NewChrootedCommandCreator(sysProcAttr *syscall.SysProcAttr) (CommandCreator, error) {

pkg/runner/local_runner_windows.go

@@ -4,56 +4,30 @@
 package runner
 
 import (
-	"context"
 	"os"
 	"os/exec"
 	"syscall"
 
 	"github.com/buildbarn/bb-remote-execution/pkg/proto/resourceusage"
-	"github.com/buildbarn/bb-storage/pkg/filesystem/path"
-	"github.com/buildbarn/bb-storage/pkg/util"
 
 	"golang.org/x/sys/windows"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/durationpb"
 )
 
-// NewPlainCommandCreator returns a CommandCreator for cases where we don't
-// need to chroot into the input root directory.
-func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator {
-	return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) {
-		// TODO: This may not work correctly if the action sets
-		// the PATH environment variable explicitly.
-		cmd := exec.CommandContext(ctx, arguments[0], arguments[1:]...)
-		cmd.SysProcAttr = sysProcAttr
-
-		// Set the working relative to be relative to the input
-		// root directory.
-		workingDirectory, scopeWalker := inputRootDirectory.Join(path.VoidScopeWalker)
-		if err := path.Resolve(workingDirectoryParser, scopeWalker); err != nil {
-			return nil, util.StatusWrap(err, "Failed to resolve working directory")
-		}
-		workingDirectoryStr, err := path.GetLocalString(workingDirectory)
-		if err != nil {
-			return nil, util.StatusWrap(err, "Failed to create local representation of working directory")
-		}
-		cmd.Dir = workingDirectoryStr
-		return cmd, nil
-	}
-}
-
 // NewChrootedCommandCreator gives an error on Windows, as chroot is not
 // supported on the platform.
 func NewChrootedCommandCreator(sysProcAttr *syscall.SysProcAttr) (CommandCreator, error) {
-	return nil, status.Error(codes.InvalidArgument, "Chroot not supported on Windows")
+	return nil, status.Error(codes.InvalidArgument, "Chroot is not supported on Windows")
 }
 
 var temporaryDirectoryEnvironmentVariablePrefixes = [...]string{"TMP=", "TEMP="}
 
 var invalidArgumentErrs = [...]error{exec.ErrNotFound, os.ErrPermission, os.ErrNotExist, windows.ERROR_BAD_EXE_FORMAT}
 
 func getPOSIXResourceUsage(cmd *exec.Cmd) *resourceusage.POSIXResourceUsage {
+	// TODO: These do not work.
 	processState := cmd.ProcessState
 	return &resourceusage.POSIXResourceUsage{
 		UserTime:   durationpb.New(processState.SystemTime()),