RSDK-12870: Properly support file:// for subsystem links.

cmd/viam-agent/main.go

@@ -81,9 +81,8 @@ func commonMain() {
 		globalLogger.SetLevel(logging.DEBUG)
 	}
 
-	if opts.UpdateFirst {
-		utils.CLIWaitForUpdateCheck = true
-	}
+	utils.CLIWaitForUpdateCheck = opts.UpdateFirst
+	utils.DevMode = opts.DevMode
 
 	// need to be root to go any further than this
 	curUser, err := user.Current()

manager.go

@@ -169,6 +169,11 @@ func (m *Manager) SelfUpdate(ctx context.Context) (bool, error) {
 		return false, ctx.Err()
 	}
 
+	// Do not manage the agent while in dev mode.
+	if utils.DevMode {
+		return false, nil
+	}
+
 	_, err := m.GetConfig(ctx)
 	if err != nil {
 		return false, err
@@ -195,17 +200,20 @@ func (m *Manager) SubsystemUpdates(ctx context.Context) {
 	m.cfgMu.Lock()
 	defer m.cfgMu.Unlock()
 
-	// Agent
-	needRestart, err := m.cache.UpdateBinary(ctx, SubsystemName)
-	if err != nil {
-		m.logger.Warn(err)
-	}
-	if needRestart {
-		_, err := InstallNewVersion(ctx, m.logger)
+	// Do not manage the agent while in dev mode.
+	if !utils.DevMode {
+		// Agent
+		needRestart, err := m.cache.UpdateBinary(ctx, SubsystemName)
 		if err != nil {
-			m.logger.Warnw("running install of new agent version", "error", err)
+			m.logger.Warn(err)
+		}
+		if needRestart {
+			_, err := InstallNewVersion(ctx, m.logger)
+			if err != nil {
+				m.logger.Warnw("running install of new agent version", "error", err)
+			}
+			m.viamAgentNeedsRestart = true
 		}
-		m.viamAgentNeedsRestart = true
 	}
 
 	// Viam Server
@@ -254,7 +262,7 @@ func (m *Manager) SubsystemUpdates(ctx context.Context) {
 			m.logger.Warn(err)
 		}
 	} else {
-		needRestart = m.sysConfig.Update(ctx, m.cfg)
+		needRestart := m.sysConfig.Update(ctx, m.cfg)
 		if needRestart {
 			if err := m.sysConfig.Stop(ctx); err != nil {
 				m.logger.Warn(err)
@@ -271,7 +279,7 @@ func (m *Manager) SubsystemUpdates(ctx context.Context) {
 			m.logger.Warn(err)
 		}
 	} else {
-		needRestart = m.networking.Update(ctx, m.cfg)
+		needRestart := m.networking.Update(ctx, m.cfg)
 		if needRestart {
 			if err := m.networking.Stop(ctx); err != nil {
 				m.logger.Warn(err)

utils/config.go

@@ -65,6 +65,7 @@ var (
 	DefaultsFilePath      = "/etc/viam-defaults.json"
 	CLIDebug              = false
 	CLIWaitForUpdateCheck = false
+	DevMode               = false
 )
 
 func init() {

utils/utils.go

@@ -160,9 +160,11 @@ func DownloadFile(ctx context.Context, rawURL string, logger logging.Logger) (st
 	logger.Infof("Starting download of %s", rawURL)
 	parsedPath := parsedURL.Path
 
-	// don't want to accidentally overwrite anything in the cache directory by accident
-	// old agent versions used a different cache format, so it's possible we could be re-downloading ourself
+	// APP-8900: Somehow, versioned binaries can be downloaded over themselves. I think when the
+	// version cache file is out of sync with what's actually on disk. I don't know why we need a
+	// version cache file.
 	var outPath string
+	outPath = filepath.Join(ViamDirs.Cache, path.Base(parsedPath))
 	for n := range 100 {
 		var suffix string
 		if n > 0 {
@@ -350,15 +352,17 @@ func DecompressFile(inPath string) (outPath string, errRet error) {
 	return outPath, errRet
 }
 
-func GetFileSum(filepath string) (outSum []byte, errRet error) {
-	//nolint:gosec
-	in, err := os.Open(filepath)
+func GetFileSum(path string) (outSum []byte, errRet error) {
+	resolvedPath, err := filepath.EvalSymlinks(path)
 	if err != nil {
 		return nil, err
 	}
-	defer func() {
-		errRet = errors.Join(errRet, in.Close())
-	}()
+
+	in, err := os.Open(resolvedPath)
+	if err != nil {
+		return nil, err
+	}
+	defer goutils.UncheckedErrorFunc(in.Close)
 
 	h := sha256.New()
 	_, errRet = io.Copy(h, in)

version_control.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
+	"net/url"
 	"os"
 	"path"
 	"path/filepath"
@@ -202,10 +203,8 @@ func (c *VersionCache) UpdateBinary(ctx context.Context, binary string) (bool, e
 		return false, errw.Errorf("unknown binary name for update request: %s", binary)
 	}
 
-	var needRestart, goodBytes bool
-
 	if data.brokenTarget {
-		return needRestart, nil
+		return false, nil
 	}
 
 	verData, ok := data.Versions[data.TargetVersion]
@@ -216,17 +215,37 @@ func (c *VersionCache) UpdateBinary(ctx context.Context, binary string) (bool, e
 		if targetVersion == "" {
 			targetVersion = "[empty string]"
 		}
-		return needRestart, errw.Errorf("version data %s not found for binary %s", targetVersion, binary)
+		return false, errw.Errorf("version data %s not found for binary %s", targetVersion, binary)
 	}
 
-	isCustomURL := strings.HasPrefix(verData.Version, "customURL+")
-	shasum, err := utils.GetFileSum(verData.UnpackedPath)
-	if err == nil {
-		goodBytes = bytes.Equal(shasum, verData.UnpackedSHA)
+	var shasum []byte
+	var shaErr error
+	urlObj, urlErr := url.Parse(verData.URL)
+	//nolint:gocritic
+	if urlErr == nil && urlObj.Scheme == "file" {
+		shasum, shaErr = utils.GetFileSum(urlObj.Path)
+	} else if urlErr == nil && urlObj.Scheme == "https" {
+		// Google storage's `x-goog-hash` header only report crc32 hashes. We could compute that
+		// from the file on disk in. But do not do so currently.
+		//
+		// Alternatively we could compare the `last-modified` header with the on disk
+		// last-modified. We do not preserve the modification time when downloading files. So we
+		// can't rely on equality. But it should also be correct to redownload anytime the in-cloud
+		// last modified is newer than the on-disk last modified. So long as the system clock can be
+		// trusted. Which may not be the case for freshly restarted raspberry pis.
+		shaErr = errors.New("we do not know how to cache invalidate custom download urls")
+	} else {
+		shasum, shaErr = utils.GetFileSum(verData.UnpackedPath)
+	}
+
+	var needRestart, bytesMatch bool
+	if shaErr == nil {
+		bytesMatch = bytes.Equal(shasum, verData.UnpackedSHA)
 	} else if verData.UnpackedPath != "" { // custom file:// URLs with have an empty unpacked path; no need to warn
-		c.logger.Warnw("Could not calculate shasum", "path", verData.UnpackedPath, "error", err)
+		c.logger.Warnw("Could not calculate shasum", "path", verData.UnpackedPath, "shaErr", shaErr)
 	}
 
+	isCustomURL := strings.HasPrefix(verData.Version, "customURL+")
 	if data.TargetVersion == data.CurrentVersion {
 		// if a known version, make sure the symlink is correct
 		same, err := utils.CheckIfSame(verData.DlPath, verData.SymlinkPath)
@@ -246,71 +265,78 @@ func (c *VersionCache) UpdateBinary(ctx context.Context, binary string) (bool, e
 			}
 		}
 
-		// TODO(APP-10012): don't leave bad checksum binary on disk + symlinked
-		if goodBytes {
-			return false, nil
-		}
-
 		// if we're here, we have a mismatched checksum, as likely the URL changed, so wipe it and recompute later
 		if isCustomURL {
 			verData.UnpackedSHA = []byte{}
 		}
 	}
 
+	// TODO(APP-10012): don't leave bad checksum binary on disk + symlinked
+	if bytesMatch {
+		return needRestart, nil
+	}
+
 	// this is a new version
 	c.logger.Infof("new version (%s) found for %s", verData.Version, binary)
+	c.logger.Warnw("mismatched checksum, redownloading",
+		"expected", hex.EncodeToString(verData.UnpackedSHA), "actual", hex.EncodeToString(shasum),
+		"url", verData.URL, "shaErr", shaErr)
 
-	if !goodBytes {
-		c.logger.Warnw("mismatched checksum, redownloading",
-			"expected", hex.EncodeToString(verData.UnpackedSHA), "actual", hex.EncodeToString(shasum),
-			"url", verData.URL)
-		// download and record the sha of the download itself
-		verData.DlPath, err = utils.DownloadFile(ctx, verData.URL, c.logger)
-		if err != nil {
-			if isCustomURL {
-				data.brokenTarget = true
-			}
-			return needRestart, errw.Wrapf(err, "downloading %s", binary)
-		}
-
-		actualSha, err := utils.GetFileSum(verData.DlPath)
-		if err != nil {
-			return needRestart, errw.Wrap(err, "getting file shasum")
+	// download and record the sha of the download itself
+	var err error
+	verData.DlPath, err = utils.DownloadFile(ctx, verData.URL, c.logger)
+	if err != nil {
+		if isCustomURL && urlObj.Scheme != "file" {
+			// If the binary was downloaded from the internet cache that link as bad forever. We
+			// don't want to redownload every five seconds to learn the binary still does not
+			// work. Even if the binary backing the URL eventually changes, unfortunately.
+			//
+			// However, if the URL represents a file on disk, we should keep retrying. Consider
+			// the case where the file was overwritten in place. Being this update logic is run
+			// every five seconds, it's common to catch a binary while its being
+			// compiled/written to.
+			data.brokenTarget = true
 		}
+		return needRestart, errw.Wrapf(err, "downloading %s", binary)
+	}
 
-		// TODO handle compressed formats, for now, the raw download is the same file
-		verData.UnpackedPath = verData.DlPath
-		verData.DlSHA = actualSha
+	actualSha, err := utils.GetFileSum(verData.DlPath)
+	if err != nil {
+		return needRestart, errw.Wrap(err, "getting file shasum")
+	}
 
-		if len(verData.UnpackedSHA) <= 1 && isCustomURL {
-			// new custom download, so need to check the file is an executable binary and use locally generated sha
-			mtype, err := mimetype.DetectFile(verData.UnpackedPath)
-			if err != nil {
-				return needRestart, errw.Wrapf(err, "determining file type of download")
-			}
+	// TODO handle compressed formats, for now, the raw download is the same file
+	verData.UnpackedPath = verData.DlPath
+	verData.DlSHA = actualSha
 
-			expectedMimes := []string{"application/x-elf", "application/x-executable"}
-			if runtime.GOOS == "windows" {
-				expectedMimes = []string{"application/vnd.microsoft.portable-executable"}
-			}
+	if len(verData.UnpackedSHA) <= 1 && isCustomURL {
+		// new custom download, so need to check the file is an executable binary and use locally generated sha
+		mtype, err := mimetype.DetectFile(verData.UnpackedPath)
+		if err != nil {
+			return needRestart, errw.Wrapf(err, "determining file type of download")
+		}
 
-			if !slices.ContainsFunc(expectedMimes, mtype.Is) {
-				data.brokenTarget = true
-				return needRestart, errw.Errorf("downloaded file is %s, not %s, skipping", mtype, strings.Join(expectedMimes, ", "))
-			}
-			verData.UnpackedSHA = actualSha
+		expectedMimes := []string{"application/x-elf", "application/x-executable"}
+		if runtime.GOOS == "windows" {
+			expectedMimes = []string{"application/vnd.microsoft.portable-executable"}
 		}
 
-		if len(verData.UnpackedSHA) > 1 && !bytes.Equal(verData.UnpackedSHA, actualSha) {
-			// TODO(APP-10012): don't leave bad checksum file on disk
-			//nolint:goerr113
-			return needRestart, fmt.Errorf(
-				"sha256 (%s) of downloaded file (%s) does not match provided (%s)",
-				base64.StdEncoding.EncodeToString(actualSha),
-				verData.UnpackedPath,
-				base64.StdEncoding.EncodeToString(verData.UnpackedSHA),
-			)
+		if !slices.ContainsFunc(expectedMimes, mtype.Is) {
+			data.brokenTarget = true
+			return needRestart, errw.Errorf("downloaded file is %s, not %s, skipping", mtype, strings.Join(expectedMimes, ", "))
 		}
+		verData.UnpackedSHA = actualSha
+	}
+
+	if len(verData.UnpackedSHA) > 1 && !bytes.Equal(verData.UnpackedSHA, actualSha) {
+		// TODO(APP-10012): don't leave bad checksum file on disk
+		//nolint:goerr113
+		return needRestart, fmt.Errorf(
+			"sha256 (%s) of downloaded file (%s) does not match provided (%s)",
+			base64.StdEncoding.EncodeToString(actualSha),
+			verData.UnpackedPath,
+			base64.StdEncoding.EncodeToString(verData.UnpackedSHA),
+		)
 	}
 
 	// chmod with execute permissions if the file is executable
@@ -320,7 +346,7 @@ func (c *VersionCache) UpdateBinary(ctx context.Context, binary string) (bool, e
 	}
 
 	// symlink the extracted file to bin
-	if err = utils.ForceSymlink(verData.UnpackedPath, verData.SymlinkPath); err != nil {
+	if err := utils.ForceSymlink(verData.UnpackedPath, verData.SymlinkPath); err != nil {
 		return needRestart, errw.Wrap(err, "creating symlink")
 	}