[botanist] Refactor

Botanist has grown unwielidy over the past ~year and so this change
returns it to a more manageable and scalable state. Much of the weight
loss is due to the removal of log support in the multi-device case, which -
interestingly - has not been used at all or relied upon since its introduction!
(Indeed, we are creating files (with %d suffixes that we have up front
not registered as task outputs, and so they never get shipped back to
the recipe).)

Also removed was botanist's care for not having streamed empty log
content. This seems like something that should more reasonably be
checked in the context of a recipe rather than something botanist should
intimately care about.

Tested: botanist is extensively integration tested being the tool that
CQ itself consumes from source to run tests and manage device
interaction. Also, this change can be viewed as bootstrapping
testability for a more streamlined botanist codebase.

Change-Id: I5809ec79e745339a4b970f8a109e2bb47fe2eef9
diff --git a/tools/botanist/BUILD.gn b/tools/botanist/BUILD.gn
index e6026d8..1ac8747 100644
--- a/tools/botanist/BUILD.gn
+++ b/tools/botanist/BUILD.gn
@@ -54,11 +54,6 @@
   deps = [ ":main" ]
 }
 
-go_test("botanist_cmd_tests") {
-  gopackages = [ "go.fuchsia.dev/fuchsia/tools/botanist/cmd" ]
-  deps = [ ":main" ]
-}
-
 go_test("botanist_lib_tests") {
   gopackages = [ "go.fuchsia.dev/fuchsia/tools/botanist/lib" ]
   deps = [ ":botanist_lib" ]
@@ -73,7 +68,6 @@
   testonly = true
 
   deps = [
-    ":botanist_cmd_tests",
     ":botanist_lib_tests",
     ":botanist_target_tests",
   ]
diff --git a/tools/botanist/cmd/run.go b/tools/botanist/cmd/run.go
index 42a2711..eb54c46 100644
--- a/tools/botanist/cmd/run.go
+++ b/tools/botanist/cmd/run.go
@@ -12,7 +12,6 @@
 	"io/ioutil"
 	"net"
 	"os"
-	"path/filepath"
 	"sync"
 	"time"
 
@@ -46,16 +45,16 @@
 	SSHKey() string
 
 	// Start starts the target.
-	Start(ctx context.Context, images []bootserver.Image, args []string) error
+	Start(context.Context, []bootserver.Image, []string) error
 
 	// Restart restarts the target.
-	Restart(ctx context.Context) error
+	Restart(context.Context) error
 
 	// Stop stops the target.
-	Stop(ctx context.Context) error
+	Stop(context.Context) error
 
 	// Wait waits for the target to finish running.
-	Wait(ctx context.Context) error
+	Wait(context.Context) error
 }
 
 // RunCommand is a Command implementation for booting a device and running a
@@ -76,12 +75,6 @@
 	// Timeout is the duration allowed for the command to finish execution.
 	timeout time.Duration
 
-	// CmdStdout is the file to which the command's stdout will be redirected.
-	cmdStdout string
-
-	// CmdStderr is the file to which the command's stderr will be redirected.
-	cmdStderr string
-
 	// SysloggerFile, if nonempty, is the file to where the system's logs will be written.
 	syslogFile string
 
@@ -121,8 +114,6 @@
 	f.BoolVar(&r.netboot, "netboot", false, "if set, botanist will not pave; but will netboot instead")
 	f.Var(&r.zirconArgs, "zircon-args", "kernel command-line arguments")
 	f.DurationVar(&r.timeout, "timeout", 10*time.Minute, "duration allowed for the command to finish execution.")
-	f.StringVar(&r.cmdStdout, "stdout", "", "file to redirect the command's stdout into; if unspecified, it will be redirected to the process' stdout")
-	f.StringVar(&r.cmdStderr, "stderr", "", "file to redirect the command's stderr into; if unspecified, it will be redirected to the process' stderr")
 	f.StringVar(&r.syslogFile, "syslog", "", "file to write the systems logs to")
 	f.StringVar(&r.sshKey, "ssh", "", "file containing a private SSH user key; if not provided, a private key will be generated.")
 	f.StringVar(&r.serialLogFile, "serial-log", "", "file to write the serial logs to.")
@@ -134,260 +125,6 @@
 	f.StringVar(&r.blobURL, "blobs", defaultBlobURL, "URL at which to serve a package repository's blobs")
 }
 
-func (r *RunCommand) runCmd(ctx context.Context, args []string, t Target) error {
-	nodename := t.Nodename()
-	ip, err := t.IPv4Addr()
-	if err == nil {
-		logger.Infof(ctx, "IPv4 address of %s found: %s", nodename, ip)
-	} else {
-		logger.Errorf(ctx, "could not resolve IPv4 address of %s: %v", nodename, err)
-	}
-
-	env := append(
-		os.Environ(),
-		fmt.Sprintf("FUCHSIA_NODENAME=%s", nodename),
-		fmt.Sprintf("FUCHSIA_IPV4_ADDR=%v", ip),
-		fmt.Sprintf("FUCHSIA_SSH_KEY=%s", t.SSHKey()),
-	)
-
-	ctx, cancel := context.WithTimeout(ctx, r.timeout)
-	defer cancel()
-
-	stdout := os.Stdout
-	if r.cmdStdout != "" {
-		f, err := os.Create(r.cmdStdout)
-		if err != nil {
-			return err
-		}
-		defer f.Close()
-		stdout = f
-	}
-	stderr := os.Stderr
-	if r.cmdStderr != "" {
-		f, err := os.Create(r.cmdStderr)
-		if err != nil {
-			return err
-		}
-		defer f.Close()
-		stderr = f
-	}
-
-	runner := runner.SubprocessRunner{
-		Env: env,
-	}
-	if err := runner.Run(ctx, args, stdout, stderr); err != nil {
-		if ctx.Err() != nil {
-			return fmt.Errorf("command timed out after %v", r.timeout)
-		}
-		return err
-	}
-	return nil
-}
-
-func getIndexedFilename(filename string, index int) string {
-	ext := filepath.Ext(filename)
-	name := filename[:len(filename)-len(ext)]
-	return fmt.Sprintf("%s-%d%s", name, index, ext)
-}
-
-type targetSetup struct {
-	targets    []Target
-	syslogs    []*logWriter
-	serialLogs []*logWriter
-	err        error
-}
-
-type logWriter struct {
-	name   string
-	file   io.ReadWriteCloser
-	target Target
-}
-
-func (r *RunCommand) setupTargets(ctx context.Context, imgs []bootserver.Image, targets []Target) *targetSetup {
-	var syslogs, serialLogs []*logWriter
-	errs := make(chan error, len(targets))
-	var wg sync.WaitGroup
-	var setupErr error
-
-	for i, t := range targets {
-		var s *os.File
-		var err error
-		if r.syslogFile != "" {
-			syslogFile := r.syslogFile
-			if len(targets) > 1 {
-				syslogFile = getIndexedFilename(r.syslogFile, i)
-			}
-			s, err = os.Create(syslogFile)
-			if err != nil {
-				setupErr = err
-				break
-			}
-			syslogs = append(syslogs, &logWriter{
-				name:   syslogFile,
-				file:   s,
-				target: t,
-			})
-		}
-
-		zirconArgs := r.zirconArgs
-		if t.Serial() != nil {
-			if r.serialLogFile != "" {
-				serialLogFile := r.serialLogFile
-				if len(targets) > 1 {
-					serialLogFile = getIndexedFilename(r.serialLogFile, i)
-				}
-				serialLog, err := os.Create(serialLogFile)
-				if err != nil {
-					setupErr = err
-					break
-				}
-				serialLogs = append(serialLogs, &logWriter{
-					name:   serialLogFile,
-					file:   serialLog,
-					target: t,
-				})
-
-				// Here we invoke the `dlog` command over serial to tail the existing log buffer into the
-				// output file.  This should give us everything since Zedboot boot, and new messages should
-				// be written to directly to the serial port without needing to tail with `dlog -f`.
-				if _, err = io.WriteString(t.Serial(), "\ndlog\n"); err != nil {
-					logger.Errorf(ctx, "failed to tail zedboot dlog: %v", err)
-				}
-
-				go func(t Target, serialLog io.ReadWriteCloser) {
-					for {
-						_, err := io.Copy(serialLog, t.Serial())
-						if err != nil && err != io.EOF {
-							logger.Errorf(ctx, "failed to write serial log: %v", err)
-							return
-						}
-					}
-				}(t, serialLog)
-				zirconArgs = append(zirconArgs, "kernel.bypass-debuglog=true")
-			}
-			// Modify the zirconArgs passed to the kernel on boot to enable serial on x64.
-			// arm64 devices should already be enabling kernel.serial at compile time.
-			zirconArgs = append(zirconArgs, "kernel.serial=legacy")
-		}
-
-		wg.Add(1)
-		go func(t Target, s *os.File, zirconArgs []string) {
-			defer wg.Done()
-			if err := t.Start(ctx, imgs, zirconArgs); err != nil {
-				errs <- err
-				return
-			}
-			nodename := t.Nodename()
-
-			if r.syslogFile != "" && s == nil {
-				errs <- fmt.Errorf("syslog is nil.")
-				return
-			}
-
-			// If having paved, SSH in and stream syslogs back to a file sink.
-			if !r.netboot && s != nil {
-				p, err := ioutil.ReadFile(t.SSHKey())
-				if err != nil {
-					errs <- err
-					return
-				}
-				config, err := sshutil.DefaultSSHConfig(p)
-				if err != nil {
-					errs <- err
-					return
-				}
-				client, err := sshutil.ConnectToNode(ctx, nodename, config)
-				if err != nil {
-					errs <- err
-					return
-				}
-				syslogger := syslog.NewSyslogger(client, config)
-				go func() {
-					defer syslogger.Close()
-					if err := syslogger.Stream(ctx, s, false); err != nil {
-						errs <- err
-					}
-				}()
-			}
-		}(t, s, zirconArgs)
-	}
-	// Wait for all targets to finish starting.
-	wg.Wait()
-	// We can close the channel on the receiver end since we wait for all target goroutines to finish.
-	close(errs)
-	err, ok := <-errs
-	if ok {
-		setupErr = err
-	}
-	return &targetSetup{
-		targets:    targets,
-		syslogs:    syslogs,
-		serialLogs: serialLogs,
-		err:        setupErr,
-	}
-}
-
-func (r *RunCommand) runCmdWithTargets(ctx context.Context, targetSetup *targetSetup, args []string) error {
-	for _, log := range targetSetup.syslogs {
-		defer log.file.Close()
-	}
-	for _, log := range targetSetup.serialLogs {
-		defer func(log *logWriter) {
-			if err := log.target.Serial().Close(); err == nil {
-				logger.Errorf(ctx, "serial output not closed yet.")
-			}
-			log.file.Close()
-		}(log)
-	}
-	for _, t := range targetSetup.targets {
-		defer func(t Target) {
-			logger.Debugf(ctx, "stopping or rebooting the node %q\n", t.Nodename())
-			if err := t.Stop(ctx); err == target.ErrUnimplemented {
-				t.Restart(ctx)
-			}
-		}(t)
-	}
-
-	if targetSetup.err != nil {
-		return targetSetup.err
-	}
-
-	errs := make(chan error)
-
-	go func() {
-		// Target doesn't matter for multi-device host tests. Just use first one.
-		errs <- r.runCmd(ctx, args, targetSetup.targets[0])
-	}()
-
-	for _, t := range targetSetup.targets {
-		go func(t Target) {
-			if err := t.Wait(ctx); err != nil && err != target.ErrUnimplemented {
-				errs <- err
-			}
-		}(t)
-	}
-
-	select {
-	case err := <-errs:
-		return err
-	case <-ctx.Done():
-	}
-	return nil
-}
-
-func checkEmptyLogs(ctx context.Context, logs []*logWriter) error {
-	for _, log := range logs {
-		b, err := ioutil.ReadFile(log.name)
-		if err != nil {
-			return err
-		}
-		if len(b) == 0 {
-			return fmt.Errorf("log is empty.")
-		}
-	}
-	return nil
-}
-
 func (r *RunCommand) execute(ctx context.Context, args []string) error {
 	var bootMode bootserver.Mode
 	if r.netboot {
@@ -417,21 +154,159 @@
 
 	var targets []Target
 	for _, obj := range objs {
-		t, err := DeriveTarget(ctx, obj, opts)
+		t, err := deriveTarget(ctx, obj, opts)
 		if err != nil {
 			return err
 		}
 		targets = append(targets, t)
 	}
+	if len(targets) == 0 {
+		return fmt.Errorf("no targets found")
+	}
+
+	// This is the primary target that a command will be run against and that
+	// logs will be streamed from.
+	t0 := targets[0]
+
+	errs := make(chan error)
+
+	serial := t0.Serial()
+	if serial != nil && r.serialLogFile != "" {
+		// Modify the zirconArgs passed to the kernel on boot to enable serial on x64.
+		// arm64 devices should already be enabling kernel.serial at compile time.
+		r.zirconArgs = append(r.zirconArgs, "kernel.serial=legacy")
+		// Force serial output to the console instead of buffering it.
+		r.zirconArgs = append(r.zirconArgs, "kernel.bypass-debuglog=true")
+
+		serialLog, err := os.Create(r.serialLogFile)
+		if err != nil {
+			return err
+		}
+		defer serialLog.Close()
+		go func() {
+			if _, err := io.Copy(serialLog, serial); err != nil {
+				errs <- fmt.Errorf("failed to write serial log: %v", err)
+			}
+		}()
+	}
 
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
-	targetSetup := r.setupTargets(ctx, imgs, targets)
-	if err := r.runCmdWithTargets(ctx, targetSetup, args); err != nil {
-		return err
+	// Defer asynchronously restarts of each target.
+	defer func() {
+		var wg sync.WaitGroup
+		for _, t := range targets {
+			wg.Add(1)
+			go func(t Target) {
+				defer wg.Done()
+				logger.Debugf(ctx, "stopping or rebooting the node %q\n", t.Nodename())
+				if err := t.Stop(ctx); err == target.ErrUnimplemented {
+					t.Restart(ctx)
+				}
+			}(t)
+		}
+		wg.Wait()
+	}()
+
+	// We wait until targets have started before running the subcommand against the zeroth one.
+	var wg sync.WaitGroup
+	for _, t := range targets {
+		wg.Add(1)
+		go func(t Target) {
+			if err := t.Start(ctx, imgs, r.zirconArgs); err != nil {
+				wg.Done()
+				errs <- err
+				return
+			}
+			wg.Done()
+			if err := t.Wait(ctx); err != nil && err != target.ErrUnimplemented {
+				errs <- err
+			}
+		}(t)
 	}
-	return checkEmptyLogs(ctx, append(targetSetup.syslogs, targetSetup.serialLogs...))
+	go func() {
+		wg.Wait()
+		errs <- r.runAgainstTarget(ctx, t0, args)
+	}()
+
+	select {
+	case err := <-errs:
+		return err
+	case <-ctx.Done():
+	}
+	return nil
+}
+
+func (r *RunCommand) runAgainstTarget(ctx context.Context, t Target, args []string) error {
+	subprocessEnv := map[string]string{
+		"FUCHSIA_NODENAME": t.Nodename(),
+	}
+
+	// If |netboot| is true, then we assume that fuchsia is not provisioned
+	// with a netstack; in this case, do not try to establish a connection.
+	if !r.netboot {
+		p, err := ioutil.ReadFile(t.SSHKey())
+		if err != nil {
+			return err
+		}
+		config, err := sshutil.DefaultSSHConfig(p)
+		if err != nil {
+			return err
+		}
+		client, err := sshutil.ConnectToNode(ctx, t.Nodename(), config)
+		if err != nil {
+			return err
+		}
+		defer client.Close()
+		subprocessEnv["FUCHSIA_SSH_KEY"] = t.SSHKey()
+
+		ip, err := t.IPv4Addr()
+		if err != nil {
+			logger.Errorf(ctx, "could not resolve IPv4 address of %s: %v", t.Nodename(), err)
+		} else if ip != nil {
+			logger.Infof(ctx, "IPv4 address of %s found: %s", t.Nodename(), ip)
+			subprocessEnv["FUCHSIA_IPV4_ADDR"] = ip.String()
+		}
+
+		if r.syslogFile != "" {
+			s, err := os.Create(r.syslogFile)
+			if err != nil {
+				return err
+			}
+			defer s.Close()
+			syslogger := syslog.NewSyslogger(client, config)
+			defer syslogger.Close()
+
+			go func() {
+				err := syslogger.Stream(ctx, s, false)
+				// TODO(fxbug.dev/43518): when 1.13 is available, spell this as
+				// `err != nil && errors.Is(err, context.Canceled) && errors.Is(err, context.DeadlineExceeded)`
+				if err != nil && ctx.Err() == nil {
+					logger.Errorf(ctx, "syslog streaming interrupted: %v", err)
+				}
+			}()
+		}
+	}
+
+	// Run the provided command against t0, adding |subprocessEnv| into
+	// its environment.
+	environ := os.Environ()
+	for k, v := range subprocessEnv {
+		environ = append(environ, fmt.Sprintf("%s=%s", k, v))
+	}
+	runner := runner.SubprocessRunner{
+		Env: environ,
+	}
+
+	ctx, cancel := context.WithTimeout(ctx, r.timeout)
+	defer cancel()
+
+	err := runner.Run(ctx, args, os.Stdout, os.Stderr)
+	if ctx.Err() == context.DeadlineExceeded {
+		return fmt.Errorf("command timed out after %v", r.timeout)
+	}
+	return err
 }
 
 func (r *RunCommand) Execute(ctx context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus {
@@ -446,7 +321,7 @@
 	return subcommands.ExitSuccess
 }
 
-func DeriveTarget(ctx context.Context, obj []byte, opts target.Options) (Target, error) {
+func deriveTarget(ctx context.Context, obj []byte, opts target.Options) (Target, error) {
 	type typed struct {
 		Type string `json:"type"`
 	}
diff --git a/tools/botanist/cmd/run_test.go b/tools/botanist/cmd/run_test.go
deleted file mode 100644
index 802945d..0000000
--- a/tools/botanist/cmd/run_test.go
+++ /dev/null
@@ -1,136 +0,0 @@
-// Copyright 2019 The Fuchsia Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package main
-
-import (
-	"context"
-	"fmt"
-	"io"
-	"io/ioutil"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-	"time"
-
-	"go.fuchsia.dev/fuchsia/tools/bootserver/lib"
-	"go.fuchsia.dev/fuchsia/tools/botanist/target"
-	"go.fuchsia.dev/fuchsia/tools/lib/color"
-	"go.fuchsia.dev/fuchsia/tools/lib/logger"
-)
-
-func TestExecute(t *testing.T) {
-	t.Run("close logs after cmd finishes", func(t *testing.T) {
-		tmpDir, err := ioutil.TempDir("", fmt.Sprintf("test-data"))
-		if err != nil {
-			t.Fatalf("failed to create temp dir: %v", err)
-		}
-		defer os.RemoveAll(tmpDir)
-		syslogPath := filepath.Join(tmpDir, "syslog.txt")
-		serialLogPath := filepath.Join(tmpDir, "serial.txt")
-
-		// create empty script to pass as cmd to run.
-		scriptPath := filepath.Join(tmpDir, "test_script.sh")
-		if err = ioutil.WriteFile(scriptPath, []byte("#!/bin/bash\n# don't do anything."), 0755); err != nil {
-			t.Fatalf("Failed to write to script: %s", err)
-		}
-
-		cmd := &RunCommand{
-			syslogFile:    syslogPath,
-			netboot:       true,
-			timeout:       time.Second * 5,
-			serialLogFile: serialLogPath,
-		}
-		loggerOutFile := filepath.Join(tmpDir, "logger_out.txt")
-		loggerOut, err := os.Create(loggerOutFile)
-		if err != nil {
-			t.Fatalf("failed to create log file: %v", err)
-		}
-
-		serialName := filepath.Join(tmpDir, "real_serial.txt")
-		serialWriter, err := os.Create(serialName)
-		if err != nil {
-			t.Fatalf("failed to create mock serial writer: %v", err)
-		}
-		defer serialWriter.Close()
-		testLogger := logger.NewLogger(logger.DebugLevel, color.NewColor(color.ColorAuto), loggerOut, loggerOut, "botanist ")
-		ctx := logger.WithLogger(context.Background(), testLogger)
-		var targets []Target
-		for i := 0; i < 2; i++ {
-			var serial io.ReadWriteCloser
-			if i == 0 {
-				serial = serialWriter
-			}
-			target, err := target.NewMockTarget(ctx, fmt.Sprintf("mock-target-%d", i+1), serial)
-			if err != nil {
-				t.Fatalf("failed to create new target: %v", err)
-			}
-			targets = append(targets, target)
-		}
-
-		// cmd.execute() calls setupTargets() followed by runCmdWithTargets().
-		// The logs should be open for writing to when runCmdWithTargets()
-		// is called and closed after it finishes.
-		targetSetup := cmd.setupTargets(ctx, []bootserver.Image{}, targets)
-		for _, log := range targetSetup.syslogs {
-			if _, err := io.WriteString(log.file, "File is open!"); err != nil {
-				t.Fatalf("File is not open: %v", err)
-			}
-		}
-		if err = cmd.runCmdWithTargets(ctx, targetSetup, []string{scriptPath}); err != nil {
-			t.Fatalf("Execute failed with error: %v", err)
-		}
-		for _, log := range targetSetup.syslogs {
-			if _, err := io.WriteString(log.file, "File is closed!"); err == nil {
-				t.Fatalf("File is open: %v", err)
-			}
-		}
-
-		loggerOut.Close()
-		log, err := ioutil.ReadFile(loggerOutFile)
-		if err != nil {
-			t.Fatalf("%v", err)
-		}
-		if strings.Contains(string(log), "serial output not closed yet") {
-			t.Fatalf("Failed to close syslog/serial logs after stopping the target.")
-		}
-	})
-
-	t.Run("fail for empty logs", func(t *testing.T) {
-		tmpDir, err := ioutil.TempDir("", fmt.Sprintf("test-data"))
-		if err != nil {
-			t.Fatalf("failed to create temp dir: %v", err)
-		}
-		defer os.RemoveAll(tmpDir)
-		syslogPath := filepath.Join(tmpDir, "syslog.txt")
-
-		// create empty script to pass as cmd to run.
-		scriptPath := filepath.Join(tmpDir, "test_script.sh")
-		if err = ioutil.WriteFile(scriptPath, []byte("#!/bin/bash\n# don't do anything."), 0755); err != nil {
-			t.Fatalf("Failed to write to script: %s", err)
-		}
-
-		cmd := &RunCommand{
-			syslogFile: syslogPath,
-			netboot:    true,
-			timeout:    time.Second * 5,
-		}
-		ctx := context.Background()
-		target, err := target.NewMockTarget(ctx, "mock-target", nil)
-		if err != nil {
-			t.Fatalf("failed to create new target: %v", err)
-		}
-		targets := []Target{target}
-
-		targetSetup := cmd.setupTargets(ctx, []bootserver.Image{}, targets)
-		if err = cmd.runCmdWithTargets(ctx, targetSetup, []string{scriptPath}); err != nil {
-			t.Fatalf("Execute failed with error: %v", err)
-		}
-		// Logs should be empty.
-		if err = checkEmptyLogs(ctx, append(targetSetup.syslogs, targetSetup.serialLogs...)); err == nil {
-			t.Fatalf("Did not fail for empty logs")
-		}
-	})
-}
diff --git a/tools/botanist/cmd/zedboot.go b/tools/botanist/cmd/zedboot.go
index b127f69..82a8b68 100644
--- a/tools/botanist/cmd/zedboot.go
+++ b/tools/botanist/cmd/zedboot.go
@@ -11,6 +11,7 @@
 	"io/ioutil"
 	"os"
 	"strings"
+	"sync"
 	"time"
 
 	"go.fuchsia.dev/fuchsia/tools/bootserver/lib"
@@ -120,58 +121,59 @@
 	defer cancel()
 	errs := make(chan error)
 
-	var serialLogs []*logWriter
-	for i, device := range devices {
-		if device.Serial() != nil {
-			if cmd.serialLogFile != "" {
-				serialLogFile := cmd.serialLogFile
-				if len(devices) > 1 {
-					serialLogFile = getIndexedFilename(cmd.serialLogFile, i)
-				}
-				serialLog, err := os.Create(serialLogFile)
-				if err != nil {
-					return err
-				}
-				serialLogs = append(serialLogs, &logWriter{
-					name: serialLogFile,
-					file: serialLog})
+	if serial := devices[0].Serial(); serial != nil && cmd.serialLogFile != "" {
+		defer serial.Close()
 
-				// Here we invoke the `dlog` command over serial to tail the existing log buffer into the
-				// output file.  This should give us everything since Zedboot boot, and new messages should
-				// be written to directly to the serial port without needing to tail with `dlog -f`.
-				if _, err = io.WriteString(device.Serial(), "\ndlog\n"); err != nil {
-					logger.Errorf(ctx, "failed to tail zedboot dlog: %v", err)
-				}
+		// Modify the zirconArgs passed to the kernel on boot to enable serial on x64.
+		// arm64 devices should already be enabling kernel.serial at compile time.
+		cmdlineArgs = append(cmdlineArgs, "kernel.serial=legacy")
+		// Force serial output to the console instead of buffering it.
+		cmdlineArgs = append(cmdlineArgs, "kernel.bypass-debuglog=true")
 
-				go func(device *target.DeviceTarget) {
-					for {
-						_, err := io.Copy(serialLog, device.Serial())
-						if err != nil && err != io.EOF {
-							logger.Errorf(ctx, "failed to write serial log: %v", err)
-							return
-						}
-					}
-				}(device)
-				cmdlineArgs = append(cmdlineArgs, "kernel.bypass-debuglog=true")
-			}
-			// Modify the cmdlineArgs passed to the kernel on boot to enable serial on x64.
-			// arm64 devices should already be enabling kernel.serial at compile time.
-			cmdlineArgs = append(cmdlineArgs, "kernel.serial=legacy")
+		serialLog, err := os.Create(cmd.serialLogFile)
+		if err != nil {
+			return err
 		}
+		defer serialLog.Close()
+		// Here we invoke the `dlog` command over serial to tail the existing log buffer into the
+		// output file.  This should give us everything since Zedboot boot, and new messages should
+		// be written to directly to the serial port without needing to tail with `dlog -f`.
+		if _, err = io.WriteString(serial, "\ndlog\n"); err != nil {
+			return fmt.Errorf("failed to tail zedboot dlog: %v", err)
+		}
+		go func() {
+			if _, err := io.Copy(serialLog, serial); err != nil {
+				errs <- fmt.Errorf("failed to write serial log: %v", err)
+			}
+		}()
+	}
 
+	// Defer asynchronously restarts of each device.
+	defer func() {
+		var wg sync.WaitGroup
+		for _, device := range devices {
+			wg.Add(1)
+			go func(device *target.DeviceTarget) {
+				defer wg.Done()
+				device.Restart(ctx)
+			}(device)
+		}
+		wg.Wait()
+	}()
+
+	var wg sync.WaitGroup
+	for _, device := range devices {
+		wg.Add(1)
 		go func(device *target.DeviceTarget) {
+			defer wg.Done()
 			if err := device.Start(ctx, imgs, cmdlineArgs); err != nil {
 				errs <- err
 			}
 		}(device)
 	}
-	for _, log := range serialLogs {
-		defer log.file.Close()
-	}
-	for _, device := range devices {
-		defer device.Restart(ctx)
-	}
+
 	go func() {
+		wg.Wait()
 		// We execute tests here against the 0th device, there may be N devices
 		// in the test bed but all other devices are driven by the tests not
 		// the test runner.
@@ -182,9 +184,8 @@
 	case err := <-errs:
 		return err
 	case <-ctx.Done():
+		return nil
 	}
-
-	return checkEmptyLogs(ctx, serialLogs)
 }
 
 func (cmd *ZedbootCommand) Execute(ctx context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus {
diff --git a/tools/botanist/target/device.go b/tools/botanist/target/device.go
index 78ef799..c69261f 100644
--- a/tools/botanist/target/device.go
+++ b/tools/botanist/target/device.go
@@ -16,7 +16,6 @@
 
 	"go.fuchsia.dev/fuchsia/tools/bootserver/lib"
 	"go.fuchsia.dev/fuchsia/tools/botanist/lib"
-	"go.fuchsia.dev/fuchsia/tools/lib/logger"
 	"go.fuchsia.dev/fuchsia/tools/net/netboot"
 	"go.fuchsia.dev/fuchsia/tools/net/netutil"
 	"go.fuchsia.dev/fuchsia/tools/net/tftp"
@@ -28,6 +27,9 @@
 const (
 	// The duration we allow for the netstack to come up when booting.
 	netstackTimeout = 90 * time.Second
+
+	// Command to dump the zircon debug log over serial.
+	dlogCmd = "\ndlog\n"
 )
 
 // DeviceConfig contains the static properties of a target device.
@@ -90,10 +92,11 @@
 	if config.Serial != "" {
 		s, err = serial.Open(config.Serial)
 		if err != nil {
-			// TODO(IN-????): This should be returned as an error, but we don't want to fail any
-			// test runs for misconfigured serial until it is actually required to complete certain
-			// tasks.
-			logger.Errorf(ctx, "unable to open %s: %v", config.Serial, err)
+			return nil, fmt.Errorf("unable to open %s: %v", config.Serial, err)
+		}
+		// Dump the existing serial debug log buffer.
+		if _, err := io.WriteString(s, dlogCmd); err != nil {
+			return nil, fmt.Errorf("failed to tail serial logs: %v", err)
 		}
 	}
 	addr, err := netutil.GetNodeAddress(ctx, config.Network.Nodename, false)
@@ -186,12 +189,12 @@
 }
 
 // Stop stops the device.
-func (t *DeviceTarget) Stop(ctx context.Context) error {
+func (t *DeviceTarget) Stop(context.Context) error {
 	return ErrUnimplemented
 }
 
 // Wait waits for the device target to stop.
-func (t *DeviceTarget) Wait(ctx context.Context) error {
+func (t *DeviceTarget) Wait(context.Context) error {
 	return ErrUnimplemented
 }