Adds a script to check for unhandled errors and fixes instances of that

Where it made sense, errors are returned (and a couple of bugs fixed).
When in a defer errors are logged if a logger is available. In a few
cases we're still ignoring the errors we get.

agent/action/fetch_logs.go

@@ -71,7 +71,9 @@ func (a FetchLogsAction) Run(logType string, filters []string) (value map[string
 		return
 	}
 
-	defer a.compressor.CleanUp(tarball)
+	defer func() {
+		_ = a.compressor.CleanUp(tarball)
+	}()
 
 	blobID, _, err := a.blobstore.Create(tarball)
 	if err != nil {

agent/action_dispatcher.go

@@ -54,7 +54,9 @@ func (dispatcher concreteActionDispatcher) ResumePreviouslyDispatchedTasks() {
 		action, err := dispatcher.actionFactory.Create(taskInfo.Method)
 		if err != nil {
 			dispatcher.logger.Error(actionDispatcherLogTag, "Unknown action %s", taskInfo.Method)
-			dispatcher.taskManager.RemoveInfo(taskInfo.TaskID)
+			if removeErr := dispatcher.taskManager.RemoveInfo(taskInfo.TaskID); removeErr != nil {
+				dispatcher.logger.Warn(actionDispatcherLogTag, "Failed to remove task info: %s", removeErr.Error())
+			}
 			continue
 		}
 

agent/agent.go

@@ -72,9 +72,19 @@ func (a Agent) Run() error {
 
 	go a.generateHeartbeats(errCh)
 
-	go a.jobSupervisor.MonitorJobFailures(a.handleJobFailure(errCh))
+	go func() {
+		err := a.jobSupervisor.MonitorJobFailures(a.handleJobFailure(errCh))
+		if err != nil {
+			errCh <- err
+		}
+	}()
 
-	go a.syslogServer.Start(a.handleSyslogMsg(errCh))
+	go func() {
+		err := a.syslogServer.Start(a.handleSyslogMsg(errCh))
+		if err != nil {
+			errCh <- err
+		}
+	}()
 
 	select {
 	case err := <-errCh:

agent/applier/jobs/rendered_job_applier.go

@@ -99,14 +99,22 @@ func (s *renderedJobApplier) downloadAndInstall(job models.Job, jobBundle boshbc
 		return bosherr.WrapError(err, "Getting temp dir")
 	}
 
-	defer s.fs.RemoveAll(tmpDir)
+	defer func() {
+		if err = s.fs.RemoveAll(tmpDir); err != nil {
+			s.logger.Warn(logTag, "Failed to clean up temp directory: %s", err.Error())
+		}
+	}()
 
 	file, err := s.blobstore.Get(job.Source.BlobstoreID, job.Source.Sha1)
 	if err != nil {
 		return bosherr.WrapError(err, "Getting job source from blobstore")
 	}
 
-	defer s.blobstore.CleanUp(file)
+	defer func() {
+		if err = s.blobstore.CleanUp(file); err != nil {
+			s.logger.Warn(logTag, "Failed to clean up blobstore blob: %s", err.Error())
+		}
+	}()
 
 	err = s.compressor.DecompressFileToDir(file, tmpDir, boshcmd.CompressorOptions{})
 	if err != nil {

agent/applier/packages/compiled_package_applier.go

@@ -92,14 +92,22 @@ func (s *compiledPackageApplier) downloadAndInstall(pkg models.Package, pkgBundl
 		return bosherr.WrapError(err, "Getting temp dir")
 	}
 
-	defer s.fs.RemoveAll(tmpDir)
+	defer func() {
+		if err = s.fs.RemoveAll(tmpDir); err != nil {
+			s.logger.Warn(logTag, "Failed to clean up tmpDir: %s", err.Error())
+		}
+	}()
 
 	file, err := s.blobstore.Get(pkg.Source.BlobstoreID, pkg.Source.Sha1)
 	if err != nil {
 		return bosherr.WrapError(err, "Fetching package blob")
 	}
 
-	defer s.blobstore.CleanUp(file)
+	defer func() {
+		if err = s.blobstore.CleanUp(file); err != nil {
+			s.logger.Warn(logTag, "Failed to clean up blobstore blog: %s", err.Error())
+		}
+	}()
 
 	err = s.compressor.DecompressFileToDir(file, tmpDir, boshcmd.CompressorOptions{})
 	if err != nil {

agent/bootstrap.go

@@ -95,7 +95,7 @@ func (boot bootstrap) Run() (err error) {
 
 	for diskID := range settings.Disks.Persistent {
 		diskSettings, _ := settings.PersistentDiskSettings(diskID)
-		if boot.platform.MountPersistentDisk(diskSettings, boot.dirProvider.StoreDir()); err != nil {
+		if err = boot.platform.MountPersistentDisk(diskSettings, boot.dirProvider.StoreDir()); err != nil {
 			return bosherr.WrapError(err, "Mounting persistent disk")
 		}
 	}

agent/cmdrunner/file_logging_cmd_runner.go

@@ -81,15 +81,19 @@ func (f FileLoggingCmdRunner) RunCommand(jobName string, taskName string, cmd bo
 	if err != nil {
 		return nil, bosherr.WrapErrorf(err, "Opening stdout for task %s", taskName)
 	}
-	defer stdoutFile.Close()
+	defer func() {
+		_ = stdoutFile.Close()
+	}()
 
 	cmd.Stdout = stdoutFile
 
 	stderrFile, err := f.fs.OpenFile(stderrPath, fileOpenFlag, fileOpenPerm)
 	if err != nil {
 		return nil, bosherr.WrapErrorf(err, "Opening stderr for task %s", taskName)
 	}
-	defer stderrFile.Close()
+	defer func() {
+		_ = stderrFile.Close()
+	}()
 
 	cmd.Stderr = stderrFile
 

agent/compiler/concrete_compiler.go

@@ -67,7 +67,9 @@ func (c concreteCompiler) Compile(pkg Package, deps []boshmodels.Package) (strin
 		return "", "", bosherr.WrapErrorf(err, "Fetching package %s", pkg.Name)
 	}
 
-	defer c.fs.RemoveAll(compilePath)
+	defer func() {
+		_ = c.fs.RemoveAll(compilePath)
+	}()
 
 	compiledPkg := boshmodels.Package{
 		Name:    pkg.Name,
@@ -115,7 +117,9 @@ func (c concreteCompiler) Compile(pkg Package, deps []boshmodels.Package) (strin
 		return "", "", bosherr.WrapError(err, "Compressing compiled package")
 	}
 
-	defer c.compressor.CleanUp(tmpPackageTar)
+	defer func() {
+		_ = c.compressor.CleanUp(tmpPackageTar)
+	}()
 
 	uploadedBlobID, sha1, err := c.blobstore.Create(tmpPackageTar)
 	if err != nil {

agentclient/http/agent_request.go

@@ -37,7 +37,9 @@ func (r agentRequest) Send(method string, arguments []interface{}, response Resp
 	if err != nil {
 		return bosherr.WrapErrorf(err, "Performing request to agent endpoint '%s'", r.endpoint)
 	}
-	defer httpResponse.Body.Close()
+	defer func() {
+		_ = httpResponse.Body.Close()
+	}()
 
 	if httpResponse.StatusCode != http.StatusOK {
 		return bosherr.Errorf("Agent responded with non-successful status code: %d", httpResponse.StatusCode)

bin/test

@@ -4,6 +4,10 @@ result=0
 
 bin=$(dirname $0)
 
+set -e
+$bin/test-unhandled-errors
+set +e
+
 $bin/test-unit -q
 let "result+=$?"
 

bin/test-unhandled-errors

@@ -0,0 +1,42 @@
+#!/bin/bash
+
+set -e
+
+echo "Checking for unhandled errors..."
+echo ""
+
+go get github.com/kisielk/errcheck
+
+packages_to_check=$(go list ./... \
+  | grep -v "github.com/cloudfoundry/bosh-agent/internal" \
+  | grep -v "github.com/cloudfoundry/bosh-agent/integration" \
+  | xargs
+)
+
+set +e
+all_errors=$(errcheck $packages_to_check)
+
+# exits 0 if no unhandled errors, 1 if there are unhandled errors, 2 if checking failed
+if [ "$?" -eq 2 ]
+then
+  echo ""
+  echo "Failed."
+  exit 1
+fi
+set -e
+
+set +e
+errors=$(echo "$all_errors" | grep -v "_test.go") # hopefully errcheck will add a flag to ignore tests and we won't have to do this: https://github.com/kisielk/errcheck/issues/66
+# exits 0 if any lines weren't excluded, 1 if all lines were excluded
+if [ "$?" -eq 1 ]
+then
+  echo "All good."
+  exit 0
+fi
+set -e
+
+num_errors=$(echo "$errors" | wc -l | awk {'print $1'})
+echo "$errors"
+echo ""
+echo "$num_errors unhandled errors found."
+exit 1

bootstrapper/auth/distinguished_names_parser.go

@@ -3,8 +3,9 @@ package auth
 import (
 	"bytes"
 	"crypto/x509/pkix"
-	"github.com/cloudfoundry/bosh-agent/internal/github.com/cloudfoundry/bosh-utils/errors"
 	"strings"
+
+	"github.com/cloudfoundry/bosh-agent/internal/github.com/cloudfoundry/bosh-utils/errors"
 )
 
 type DistinguishedNamesParser interface {
@@ -54,7 +55,9 @@ func (parser distinguishedNamesParser) Parse(dn string) (*pkix.Name, error) {
 
 	for _, c := range dn {
 		if escape {
-			buf.WriteRune(c)
+			if _, err := buf.WriteRune(c); err != nil {
+				return nil, err
+			}
 			escape = false
 		} else if c == '=' {
 			ident = buf.String()
@@ -70,7 +73,9 @@ func (parser distinguishedNamesParser) Parse(dn string) (*pkix.Name, error) {
 		} else if c == '\\' {
 			escape = true
 		} else {
-			buf.WriteRune(c)
+			if _, err := buf.WriteRune(c); err != nil {
+				return nil, err
+			}
 		}
 	}
 

bootstrapper/listener/listener.go

@@ -79,7 +79,7 @@ func (l *Listener) ListenAndServe(logger logger.Logger, port int) error {
 func (l *Listener) Close() {
 	if l.started {
 		l.closing = true
-		l.listener.Close()
+		_ = l.listener.Close()
 		l.started = false
 	}
 }

bootstrapper/listener/self_update_handler.go

@@ -29,7 +29,9 @@ func (h *SelfUpdateHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request)
 		} else {
 			rw.WriteHeader(StatusUnprocessableEntity)
 		}
-		rw.Write([]byte(err.Error()))
+		if _, wErr := rw.Write([]byte(err.Error())); wErr != nil {
+			h.Logger.Warn("SelfUpdateHandler", "Failed to write error to buffer: %s", wErr.Error())
+		}
 		h.Logger.Error("SelfUpdateHandler", "failed to install package: %s", err.Error())
 		return
 	}

bootstrapper/spec/download_server.go

@@ -38,16 +38,20 @@ func StartDownloadServer(port int, tarballPath string, directorCert *tls.Certifi
 		Expect(err).ToNot(HaveOccurred())
 		_, err = io.Copy(w, tarReader)
 		Expect(err).ToNot(HaveOccurred())
-		tarReader.Close()
+		err = tarReader.Close()
+		Expect(err).ToNot(HaveOccurred())
 	})
-	go server.Serve(tlsListener)
+	go func() {
+		_ = server.Serve(tlsListener)
+	}()
 	return tlsListener
 }
 
 func CreateTarball(installScript string) string {
 	tmpDir, err := ioutil.TempDir("", "test-tmp")
 	Expect(err).ToNot(HaveOccurred())
-	ioutil.WriteFile(path.Join(tmpDir, "install.sh"), ([]byte)(installScript), 0755)
+	err = ioutil.WriteFile(path.Join(tmpDir, "install.sh"), ([]byte)(installScript), 0755)
+	Expect(err).ToNot(HaveOccurred())
 	tarCmd := exec.Command("tar", "cfz", "tarball.tgz", "install.sh")
 	tarCmd.Dir = tmpDir
 	_, err = tarCmd.CombinedOutput()
@@ -61,6 +65,7 @@ func GetFreePort() int {
 	Expect(err).ToNot(HaveOccurred())
 
 	port := listener.Addr().(*net.TCPAddr).Port
-	listener.Close()
+	err = listener.Close()
+	Expect(err).ToNot(HaveOccurred())
 	return port
 }

bootstrapper/system/os_system.go

@@ -47,7 +47,10 @@ func (system *osSystem) RunScript(scriptPath string, workingDir string) (Command
 		return CommandResult{}, err
 	}
 
-	io.Copy(os.Stdout, out)
+	_, err = io.Copy(os.Stdout, out)
+	if err != nil {
+		return CommandResult{}, err
+	}
 
 	exitStatus := getExitStatus(command.Wait())
 	return CommandResult{

httpsdispatcher/https_dispatcher.go

@@ -80,7 +80,7 @@ func (h *HTTPSDispatcher) Start() error {
 
 func (h *HTTPSDispatcher) Stop() {
 	if h.listener != nil {
-		h.listener.Close()
+		_ = h.listener.Close()
 		h.listener = nil
 	}
 }

infrastructure/http_metadata_service.go

@@ -51,7 +51,11 @@ func (ms httpMetadataService) GetPublicKey() (string, error) {
 		return "", bosherr.WrapError(err, "Getting open ssh key")
 	}
 
-	defer resp.Body.Close()
+	defer func() {
+		if err := resp.Body.Close(); err != nil {
+			ms.logger.Warn(ms.logTag, "Failed to close response body when getting ssh key: %s", err.Error())
+		}
+	}()
 
 	bytes, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
@@ -73,7 +77,11 @@ func (ms httpMetadataService) GetInstanceID() (string, error) {
 		return "", bosherr.WrapError(err, "Getting instance id from url")
 	}
 
-	defer resp.Body.Close()
+	defer func() {
+		if err := resp.Body.Close(); err != nil {
+			ms.logger.Warn(ms.logTag, "Failed to close response body when getting instance id: %s", err.Error())
+		}
+	}()
 
 	bytes, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
@@ -137,7 +145,11 @@ func (ms httpMetadataService) getUserData() (UserDataContentsType, error) {
 		return userData, bosherr.WrapError(err, "Getting user data from url")
 	}
 
-	defer userDataResp.Body.Close()
+	defer func() {
+		if err := userDataResp.Body.Close(); err != nil {
+			ms.logger.Warn(ms.logTag, "Failed to close response body when getting user data: %s", err.Error())
+		}
+	}()
 
 	userDataBytes, err := ioutil.ReadAll(userDataResp.Body)
 	if err != nil {

infrastructure/http_registry.go

@@ -74,7 +74,9 @@ func (r httpRegistry) GetSettings() (boshsettings.Settings, error) {
 		return settings, bosherr.WrapError(err, "Getting settings from url")
 	}
 
-	defer wrapperResponse.Body.Close()
+	defer func() {
+		_ = wrapperResponse.Body.Close()
+	}()
 
 	wrapperBytes, err := ioutil.ReadAll(wrapperResponse.Body)
 	if err != nil {

jobsupervisor/alert_envelope.go

@@ -49,7 +49,10 @@ func (e *alertEnvelope) Close() error {
 	emptyAlert := boshalert.MonitAlert{}
 
 	if alertToHandle != emptyAlert {
-		e.handler(*e.alert)
+		err := e.handler(*e.alert)
+		if err != nil {
+			return err
+		}
 	}
 
 	return nil

jobsupervisor/dummy_nats_job_supervisor.go

@@ -70,7 +70,7 @@ func (d *dummyNatsJobSupervisor) statusHandler(req boshhandler.Request) boshhand
 		d.status = body["status"]
 
 		if d.status == "failing" && d.jobFailureHandler != nil {
-			d.jobFailureHandler(boshalert.MonitAlert{
+			_ = d.jobFailureHandler(boshalert.MonitAlert{
 				ID:          "fake-monit-alert",
 				Service:     "fake-monit-service",
 				Event:       "failing",

jobsupervisor/fakes/fake_job_supervisor.go

@@ -79,7 +79,7 @@ func (m *FakeJobSupervisor) Status() string {
 
 func (m *FakeJobSupervisor) MonitorJobFailures(handler boshjobsuper.JobFailureHandler) error {
 	if m.JobFailureAlert != nil {
-		handler(*m.JobFailureAlert)
+		return handler(*m.JobFailureAlert)
 	}
 	return nil
 }