Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dgottlieb wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions cmd/viam-agent/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
30 changes: 19 additions & 11 deletions manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
1 change: 1 addition & 0 deletions utils/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ var (
DefaultsFilePath = "/etc/viam-defaults.json"
CLIDebug = false
CLIWaitForUpdateCheck = false
DevMode = false
Copy link
Member Author

@dgottlieb dgottlieb Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

driveby

)

func init() {
Expand Down
20 changes: 12 additions & 8 deletions utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Copy link
Member

@cheukt cheukt Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we were just getting the checksum of the symlink before? and that's why file:// links weren't triggering a checksum mismatch and redownload?

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)
Expand Down
142 changes: 84 additions & 58 deletions version_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"errors"
"fmt"
"io/fs"
"net/url"
"os"
"path"
"path/filepath"
Expand Down Expand Up @@ -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]
Expand All @@ -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)
Expand All @@ -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
Copy link
Member Author

@dgottlieb dgottlieb Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brokenTarget logic I changed. The "re-check subsystems to see if things changed" happens every 5 seconds. I was routinely hitting the window where a viam-server recompile removed the old file, but had not yet written out the new one.

}
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)
Copy link
Member Author

@dgottlieb dgottlieb Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else here should be exactly the same except for the level of indentation.

I've misread this code -- trying again

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
Expand All @@ -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")
}

Expand Down
Loading