state, internal/dao, util: revert hot-path fsync, fix leak and doc inaccuracy

- util.AtomicWriteFile: drop the fsync added last round. On macOS, Go's
  File.Sync issues fcntl(F_FULLFSYNC), which measured ~140x slower than the
  plain write here (confirmed on this machine: ~4.3ms vs ~30µs/op) -- and this
  helper runs on Hermit's "exec" hot path via dao.UpdatePackage. The data it
  protects is a regenerable cache (etag + check timestamp), so losing it to a
  crash just costs one extra upstream check; that's not worth paying this cost
  on every invocation. Documented as a deliberate omission.
- state.extract: the "copy manifest referred files" loop can also fail after
  archive.Extract has already published p.Dest, the same condition the
  previous commit fixed for the EventUnpack trigger a few lines below it --
  missed because it wasn't the line called out by review. Now cleaned up with
  util.RemoveAllAtomic here too, otherwise a retry of the same package is
  permanently wedged behind archive.Extract's "destination already exists".
- internal/dao.UpdatePackage: correct a doc comment claiming the etag/
  checked-at interleave risk was "not introduced by the two-file split" --
  the single-JSON-file format it replaced wrote both fields in one atomic
  rename, so this specific mismatched pairing is in fact newly possible.
  Still benign: worst case is a one-cycle-stale check time that self-corrects.

Author: jason-rl

internal/dao/dao.go

@@ -140,10 +140,12 @@ func (d *DAO) readCheckedAt(pkgRef string) (time.Time, error) {
 // so two UpdatePackage calls for the same package racing each other can
 // interleave: caller A's etag write can be immediately followed by caller
 // B's checked-at write, leaving a GetPackage that reads in between with A's
-// etag paired with B's checked-at time. This is a pre-existing risk carried
-// over from the single-JSON-file format this replaced (which had the same
-// last-writer-wins exposure across the two logical fields, just within one
-// file); it is not introduced by the two-file split.
+// etag paired with B's checked-at time. The single-JSON-file format this
+// replaced wrote both fields in one atomic rename, so this specific
+// mismatched pairing is newly possible with the two-file split -- but it's
+// still benign: at worst it under- or over-estimates how recently a
+// concurrently-updated package was checked by one update cycle, which
+// self-corrects on the next check.
 func (d *DAO) UpdatePackage(pkgRef string, pkg *Package) error {
 	checkedAt := pkg.UpdateCheckedAt
 	if checkedAt.IsZero() {

state/state.go

@@ -454,18 +454,22 @@ func (s *State) extract(b *ui.Task, p *manifest.Package) error {
 	if err != nil {
 		return errors.WithStack(err)
 	}
+	// From here on, p.Dest is already published: archive.Extract renames it
+	// into place before returning, not after finalise() runs. That means an
+	// unlocked reader could be looking at it, and archive.Extract itself
+	// refuses to extract into a p.Dest that already exists -- so any failure
+	// below must clean it up the same reader-safe way as everywhere else in
+	// this package, or a retry of this package is permanently wedged behind
+	// "destination already exists".
 	// Copy manifest referred files
 	for _, file := range p.Files {
 		err = vfs.CopyFile(file.FS, file.FromPath, file.ToPath)
 		if err != nil {
+			_ = util.RemoveAllAtomic(p.Dest)
 			return errors.WithStack(err)
 		}
 	}
 	if _, err = p.Trigger(b, manifest.EventUnpack); err != nil {
-		// p.Dest is already published (archive.Extract renames it into place
-		// before returning), so an unlocked reader could be looking at it --
-		// remove it the same reader-safe way as everywhere else in this
-		// package rather than deleting it out from under them entry by entry.
 		_ = util.RemoveAllAtomic(p.Dest)
 		return errors.WithStack(err)
 	}

util/atomicfile.go

@@ -11,6 +11,17 @@ import (
 // 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.
+//
+// This deliberately does not fsync the temp file before renaming it: doing so
+// closes a narrow crash-durability gap (a crash between a successful-looking
+// write and the underlying data actually reaching disk could otherwise leave
+// path pointing at a zero-length or truncated file), but on macOS, Go's
+// File.Sync issues fcntl(F_FULLFSYNC), which is roughly two orders of
+// magnitude slower than a plain write -- and this is called from
+// internal/dao.UpdatePackage on Hermit's "exec" hot path. The data this
+// protects (a cached etag and check timestamp) is not authoritative state:
+// losing it to a crash just costs one extra upstream check on the next run,
+// which doesn't justify that cost on every invocation.
 func AtomicWriteFile(path string, data []byte, perm os.FileMode) error {
 	dir := filepath.Dir(path)
 	tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*")
@@ -22,20 +33,10 @@ func AtomicWriteFile(path string, data []byte, perm os.FileMode) error {
 	defer os.Remove(tmpPath)
 
 	_, writeErr := tmp.Write(data)
-	var syncErr error
-	if writeErr == nil {
-		// Without this, a crash shortly after Rename can leave path pointing
-		// at a temp file the filesystem never flushed, ie. a zero-length or
-		// truncated file, despite the rename itself being durable.
-		syncErr = tmp.Sync()
-	}
 	closeErr := tmp.Close()
 	if writeErr != nil {
 		return errors.WithStack(writeErr)
 	}
-	if syncErr != nil {
-		return errors.WithStack(syncErr)
-	}
 	if closeErr != nil {
 		return errors.WithStack(closeErr)
 	}