internal/dao, state, env: drop JSON envelope, add shared atomic-write helper

High: the etag metadata file's JSON envelope required parsing on every
GetPackage, and a torn or short read (concurrent UpdatePackage without
this envelope's atomicity) parsed as garbage that UpgradeChannel would
treat as a genuine etag change, triggering evictPackage's rm -rf of a
package tree that may be actively executing. Drop the envelope
entirely: the etag is now the exact raw bytes every Hermit version has
always written, stored via the new shared util.AtomicWriteFile;
UpdateCheckedAt moves to a separate ".checked" sidecar file so mixed
Hermit versions sharing a state directory keep reading and writing the
etag identically, with graceful fallback to the etag file's mtime when
the sidecar is missing or unparseable.

Medium: sweep stale ".tmp-*" scratch files left behind by a killed
process (whose deferred cleanup never got to run) out of the metadata
directory on DAO Open, bounded by a generous age threshold so a
genuinely in-flight write from another process is never touched.
Extend the same atomic-write treatment to env.go's SetEnv/DelEnv, and
give state.removeRecursive an atomic (rename-aside) removal via the new
util.RemoveAllAtomic, matching the reader-visible-window fix util.SwapDir
already applies to replacement.

Also documents that WritePackageState storing a zero UpdateCheckedAt as
"now" (via dao.UpdatePackage) is harmless when UpdateInterval == 0,
since EnsureChannelIsUpToDate short-circuits before ever consulting it.

Author: jason-rl

env.go

@@ -1211,7 +1211,7 @@ func (e *Env) SetEnv(key, value string) error {
 	if err != nil {
 		return errors.WithStack(err)
 	}
-	return os.WriteFile(e.configFile, data, 0600)
+	return errors.WithStack(util.AtomicWriteFile(e.configFile, data, 0600))
 }
 
 // DelEnv deletes a custom environment variable.
@@ -1221,7 +1221,7 @@ func (e *Env) DelEnv(key string) error {
 	if err != nil {
 		return errors.WithStack(err)
 	}
-	return os.WriteFile(e.configFile, data, 0600)
+	return errors.WithStack(util.AtomicWriteFile(e.configFile, data, 0600))
 }
 
 // Clean parts of the hermit system.

internal/dao/dao.go

@@ -1,15 +1,21 @@
 package dao
 
 import (
-	"encoding/json"
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/cashapp/hermit/errors"
+	"github.com/cashapp/hermit/util"
 )
 
+// staleScratchAge is how old a leftover ".tmp-*" file must be before Open
+// considers it abandoned rather than an in-flight write from another
+// process.
+const staleScratchAge = 24 * time.Hour
+
 // DAO abstracts away the database access
 type DAO struct {
 	stateDir    string
@@ -28,109 +34,135 @@ func Open(stateDir string) (*DAO, error) {
 	if err := os.MkdirAll(metadataDir, 0700); err != nil && !os.IsExist(err) {
 		return nil, errors.WithStack(err)
 	}
+	sweepStaleScratchFiles(metadataDir)
 	return &DAO{stateDir: stateDir, metadataDir: metadataDir}, nil
 }
 
+// sweepStaleScratchFiles removes leftover ".tmp-*" files from
+// util.AtomicWriteFile calls that were interrupted by a killed process (eg.
+// SIGKILL, which the writer's deferred os.Remove cannot run for). Best
+// effort: errors are ignored, and a generous age threshold avoids racing a
+// concurrent, genuinely in-flight write from another Hermit process.
+func sweepStaleScratchFiles(metadataDir string) {
+	entries, err := os.ReadDir(metadataDir)
+	if err != nil {
+		return
+	}
+	for _, entry := range entries {
+		if !strings.Contains(entry.Name(), ".tmp-") {
+			continue
+		}
+		info, err := entry.Info()
+		if err != nil || time.Since(info.ModTime()) < staleScratchAge {
+			continue
+		}
+		_ = os.Remove(filepath.Join(metadataDir, entry.Name()))
+	}
+}
+
 // Dump content of database to w.
 func (d *DAO) Dump(w io.Writer) error {
 	return nil
 }
 
-// metadataFile is the on-disk encoding of Package written by UpdatePackage.
-//
-// UpdateCheckedAt is stored explicitly, rather than inferred from the file's
-// mtime (as earlier versions of Hermit did): mtime can't be trusted to mean
-// "the moment this etag was written" -- it's disturbed by anything else that
-// touches the file (eg. a backup/restore), and differs in precision across
-// filesystems.
-type metadataFile struct {
-	Etag            string    `json:"etag"`
-	UpdateCheckedAt time.Time `json:"update_checked_at"`
-}
-
 // GetPackage returns information for a specific package.
+//
+// The etag is stored as the raw, unencoded file content at metadataPath: this
+// is the exact on-disk format every Hermit version has ever written, so a
+// mixed-version fleet sharing a state directory can always read and write it
+// identically. UpdateCheckedAt is stored separately, in the sidecar file at
+// checkedAtPath, because mtime can't be trusted to mean "the moment this etag
+// was written" -- it's disturbed by anything else that touches the file (eg.
+// a backup/restore), and differs in precision across filesystems. An older
+// Hermit version, or a first-ever check, has no such sidecar: fall back to
+// the etag file's mtime in that case, as GetPackage always did previously.
 func (d *DAO) GetPackage(pkgRef string) (*Package, error) {
-	r, err := os.Open(d.metadataPath(pkgRef))
+	etag, err := os.ReadFile(d.metadataPath(pkgRef))
 	if os.IsNotExist(err) {
 		return nil, nil
 	}
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
-	defer r.Close()
-	info, err := r.Stat()
+	checkedAt, err := d.readCheckedAt(pkgRef)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
-	data, err := io.ReadAll(r)
-	if err != nil {
-		return nil, errors.WithStack(err)
-	}
-	var mf metadataFile
-	if err := json.Unmarshal(data, &mf); err != nil {
-		// Metadata file written by a Hermit version prior to the
-		// introduction of this format: it contains only the raw etag, with
-		// no recorded check time. Fall back to the file's mtime, as
-		// GetPackage always did previously.
-		return &Package{
-			Etag:            string(data),
-			UpdateCheckedAt: info.ModTime(),
-		}, nil
+	if checkedAt.IsZero() {
+		info, err := os.Stat(d.metadataPath(pkgRef))
+		if err != nil {
+			return nil, errors.WithStack(err)
+		}
+		checkedAt = info.ModTime()
 	}
 	return &Package{
-		Etag:            mf.Etag,
-		UpdateCheckedAt: mf.UpdateCheckedAt,
+		Etag:            string(etag),
+		UpdateCheckedAt: checkedAt,
 	}, nil
 }
 
-// UpdatePackage updates the update check time, etag, and the used at time for a package.
+func (d *DAO) readCheckedAt(pkgRef string) (time.Time, error) {
+	data, err := os.ReadFile(d.checkedAtPath(pkgRef))
+	if os.IsNotExist(err) {
+		return time.Time{}, nil
+	}
+	if err != nil {
+		return time.Time{}, errors.WithStack(err)
+	}
+	checkedAt, err := time.Parse(time.RFC3339Nano, string(data))
+	if err != nil {
+		// A torn read of the sidecar (or one written by an incompatible
+		// future version) is not fatal: fall back to mtime rather than
+		// failing the whole lookup.
+		return time.Time{}, nil //nolint:nilerr
+	}
+	return checkedAt, nil
+}
+
+// UpdatePackage updates the update check time and etag for a package.
 //
-// The write is atomic: content is written to a temp file in the same
-// directory, then renamed into place. os.WriteFile is not atomic -- it
-// truncates the existing file before writing the new content -- so a
+// Both files are written atomically: content is written to a temp file in
+// the same directory, then renamed into place. os.WriteFile is not atomic --
+// it truncates the existing file before writing the new content -- so a
 // concurrent GetPackage could otherwise observe a torn read (empty or
 // partial etag). A torn read here is not merely cosmetic: UpgradeChannel
 // treats any etag change, including a corrupted one, as a reason to
 // evictPackage (rm -rf) a package tree that another process may be actively
 // executing.
+//
+// The etag is written first: if the process dies between the two writes, a
+// concurrent GetPackage falls back to the etag file's mtime for
+// UpdateCheckedAt (see above), which is the same degraded-but-safe behaviour
+// as running against an older Hermit version that never writes the sidecar
+// at all.
 func (d *DAO) UpdatePackage(pkgRef string, pkg *Package) error {
-	path := d.metadataPath(pkgRef)
 	checkedAt := pkg.UpdateCheckedAt
 	if checkedAt.IsZero() {
 		checkedAt = time.Now()
 	}
-	data, err := json.Marshal(metadataFile{Etag: pkg.Etag, UpdateCheckedAt: checkedAt})
-	if err != nil {
+	if err := util.AtomicWriteFile(d.metadataPath(pkgRef), []byte(pkg.Etag), 0600); err != nil {
 		return errors.WithStack(err)
 	}
-
-	tmp, err := os.CreateTemp(d.metadataDir, filepath.Base(path)+".tmp-*")
-	if err != nil {
-		return errors.WithStack(err)
-	}
-	tmpPath := tmp.Name()
-	// Harmless once the rename below succeeds: nothing left to remove.
-	defer os.Remove(tmpPath)
-
-	_, writeErr := tmp.Write(data)
-	closeErr := tmp.Close()
-	if writeErr != nil {
-		return errors.WithStack(writeErr)
-	}
-	if closeErr != nil {
-		return errors.WithStack(closeErr)
-	}
-	return errors.WithStack(os.Rename(tmpPath, path))
+	return errors.WithStack(util.AtomicWriteFile(d.checkedAtPath(pkgRef), []byte(checkedAt.Format(time.RFC3339Nano)), 0600))
 }
 
 // DeletePackage removes a package from the DB
 func (d *DAO) DeletePackage(pkgRef string) error {
 	if err := os.Remove(d.metadataPath(pkgRef)); err != nil {
 		return errors.WithStack(err)
 	}
+	// The checked-at sidecar may not exist (eg. written by an older Hermit
+	// version); that's not an error.
+	if err := os.Remove(d.checkedAtPath(pkgRef)); err != nil && !os.IsNotExist(err) {
+		return errors.WithStack(err)
+	}
 	return nil
 }
 
 func (d *DAO) metadataPath(pkgRef string) string {
 	return filepath.Join(d.metadataDir, pkgRef+".etag")
 }
+
+func (d *DAO) checkedAtPath(pkgRef string) string {
+	return filepath.Join(d.metadataDir, pkgRef+".checked")
+}

internal/dao/dao_test.go

@@ -37,12 +37,15 @@ func TestUpdateAndGetPackageRoundTrip(t *testing.T) {
 	assert.True(t, checkedAt.Equal(got.UpdateCheckedAt), "expected %s, got %s", checkedAt, got.UpdateCheckedAt)
 }
 
-// TestGetPackageLegacyFormat verifies that a metadata file written by a
-// Hermit version prior to the introduction of the JSON envelope (ie. one
-// containing only the raw etag, with no recorded check time) is still read
-// correctly, falling back to the file's mtime for UpdateCheckedAt exactly as
-// GetPackage always did previously.
-func TestGetPackageLegacyFormat(t *testing.T) {
+// TestGetPackageMissingCheckedAtSidecar verifies that a metadata directory
+// containing only the raw etag file, with no ".checked" sidecar, is still
+// read correctly, falling back to the etag file's mtime for
+// UpdateCheckedAt. This is the on-disk state left by a Hermit version prior
+// to the introduction of the sidecar (which wrote only the raw etag, in
+// exactly this format) -- the two are indistinguishable, which is the point:
+// an older Hermit binary sharing this state directory can still read and
+// write the etag file unmodified.
+func TestGetPackageMissingCheckedAtSidecar(t *testing.T) {
 	d, err := Open(t.TempDir())
 	assert.NoError(t, err)
 
@@ -112,8 +115,36 @@ func TestUpdatePackageAtomicNoTornRead(t *testing.T) {
 	assert.NoError(t, <-readerErr)
 }
 
+// TestOpenSweepsStaleScratchFiles verifies that Open cleans up an old,
+// abandoned ".tmp-*" file left behind by a process killed mid-write, but
+// leaves a recent one alone (it may belong to a write still in flight in
+// another process).
+func TestOpenSweepsStaleScratchFiles(t *testing.T) {
+	stateDir := t.TempDir()
+	metadataDir := filepath.Join(stateDir, "metadata")
+	assert.NoError(t, os.MkdirAll(metadataDir, 0700))
+
+	stale := filepath.Join(metadataDir, "pkg@1.0.0.etag.tmp-stale")
+	assert.NoError(t, os.WriteFile(stale, []byte("abandoned"), 0600))
+	old := time.Now().Add(-48 * time.Hour)
+	assert.NoError(t, os.Chtimes(stale, old, old))
+
+	fresh := filepath.Join(metadataDir, "pkg@2.0.0.etag.tmp-fresh")
+	assert.NoError(t, os.WriteFile(fresh, []byte("in-flight"), 0600))
+
+	_, err := Open(stateDir)
+	assert.NoError(t, err)
+
+	_, err = os.Stat(stale)
+	assert.True(t, os.IsNotExist(err), "stale scratch file should have been swept")
+	_, err = os.Stat(fresh)
+	assert.NoError(t, err, "recent scratch file should not have been swept")
+}
+
 // TestUpdatePackageLeavesNoTempFiles guards against leaking the scratch temp
-// file UpdatePackage writes before renaming into place.
+// files UpdatePackage writes before renaming into place -- there are two
+// atomic writes per call (the ".etag" file and the ".checked" sidecar), each
+// with its own temp file.
 func TestUpdatePackageLeavesNoTempFiles(t *testing.T) {
 	stateDir := t.TempDir()
 	d, err := Open(stateDir)
@@ -123,9 +154,12 @@ func TestUpdatePackageLeavesNoTempFiles(t *testing.T) {
 
 	entries, err := os.ReadDir(filepath.Join(stateDir, "metadata"))
 	assert.NoError(t, err)
+	var names []string
 	for _, entry := range entries {
 		if strings.Contains(entry.Name(), ".tmp-") {
 			t.Fatalf("leaked temp file: %s", entry.Name())
 		}
+		names = append(names, entry.Name())
 	}
+	assert.Equal(t, []string{"pkg@1.0.0.checked", "pkg@1.0.0.etag"}, names)
 }

state/state.go

@@ -262,6 +262,13 @@ func (s *State) ReadPackageState(pkg *manifest.Package) {
 }
 
 // WritePackageState updates the fields and usage time stamp of the given package
+//
+// A zero UpdateCheckedAt (when p.UpdateInterval <= 0, ie. this package never
+// checks for updates) is stored as "now" by dao.UpdatePackage rather than as
+// a literal zero time -- see its docs. That's harmless here specifically:
+// EnsureChannelIsUpToDate short-circuits on UpdateInterval == 0 before ever
+// consulting UpdatedAt, so the substituted value is never read back for a
+// package in this state.
 func (s *State) WritePackageState(p *manifest.Package) error {
 	updatedAt := time.Time{}
 	if p.UpdateInterval > 0 {
@@ -304,7 +311,7 @@ func (s *State) removeRecursive(b *ui.Task, dest string) error {
 		return errors.WithStack(err)
 	})
 	task.Debugf("rm -rf %s", dest)
-	return errors.WithStack(os.RemoveAll(dest))
+	return errors.WithStack(util.RemoveAllAtomic(dest))
 }
 
 // CacheAndUnpack downloads a package and extracts it if it is not present.

util/atomicfile.go

@@ -0,0 +1,36 @@
+package util
+
+import (
+	"os"
+	"path/filepath"
+
+	"github.com/cashapp/hermit/errors"
+)
+
+// AtomicWriteFile writes data to path atomically: it is written to a temp
+// file in the same directory, then renamed into place. Unlike os.WriteFile,
+// which truncates the existing file before writing, a concurrent reader can
+// never observe an empty or partially-written file.
+func AtomicWriteFile(path string, data []byte, perm os.FileMode) error {
+	dir := filepath.Dir(path)
+	tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*")
+	if err != nil {
+		return errors.WithStack(err)
+	}
+	tmpPath := tmp.Name()
+	// Harmless once the rename below succeeds: nothing left to remove.
+	defer os.Remove(tmpPath)
+
+	_, writeErr := tmp.Write(data)
+	closeErr := tmp.Close()
+	if writeErr != nil {
+		return errors.WithStack(writeErr)
+	}
+	if closeErr != nil {
+		return errors.WithStack(closeErr)
+	}
+	if err := os.Chmod(tmpPath, perm); err != nil {
+		return errors.WithStack(err)
+	}
+	return errors.WithStack(os.Rename(tmpPath, path))
+}