internal/dao, state: fix two more torn-write races on the hermit-exec hot path (4/4) - #592
Open
jason-rl wants to merge 15 commits into
Open
internal/dao, state: fix two more torn-write races on the hermit-exec hot path (4/4)#592jason-rl wants to merge 15 commits into
jason-rl wants to merge 15 commits into
Conversation
Executing a Hermit-managed binary that hasn't been installed yet, several times within a few milliseconds of each other, can make some invocations fail with "unknown package" even though the package is perfectly valid. GitSource.Sync has no cross-process or cross-goroutine locking, so concurrent syncs of the same not-yet-cloned source race: each clones independently and then wipes and replaces the shared manifest tree, leaving a window where a concurrent reader sees ENOENT partway through. TestConcurrentSyncInProcess and TestConcurrentSyncAcrossProcesses reproduce this directly (the latter across genuine child processes, since util/flock is deliberately re-entrant per-PID and so cannot exercise cross-process contention from goroutines alone). Both fail against the current implementation; the fix follows in a subsequent change.
3 tasks
jason-rl
changed the title
TestConcurrentSyncInProcess and TestConcurrentSyncAcrossProcesses previously let goroutines/child processes begin racing as soon as each was spawned, so on a fast machine some finished before the last one even started, understating how often the race actually reproduces. Hold every goroutine/child at a barrier until all have signalled ready, then release them together, so all n consistently race through Sync concurrently. Also corrects TestConcurrentSyncInProcess's doc comment, which overclaimed that -race specifically exercises "the process-local mutex in sources/lock.go" -- that file doesn't exist yet at this point in the stack.
Fixes the race reproduced in the previous commit. GitSource.Sync had no cross-process or cross-goroutine locking, so concurrent syncs of the same not-yet-cloned source raced: every caller passed the same pre-lock check, cloned independently, and each then did RemoveAll(dest) + Rename(tmp, dest) to install its result -- an unlink storm over the whole manifest tree that any concurrent reader could observe mid-way through as ENOENT, which is exactly the "unknown package" failure this was reported as. - sources/lock.go adds acquireSyncLock: a cross-process flock plus a process-local sync.Mutex (needed because util/flock is deliberately re-entrant per-PID, so it's a no-op between goroutines of the same process). - GitSource.Sync now takes this lock around the whole sync, with double-checked locking against the pre/post-lock mtime so a waiter that loses the race skips redundant work, and degrades to the existing copy (rather than failing) if the lock can't be acquired in time and a usable tree already exists. - The install step no longer destroys the target before the new tree is ready: util.SwapDir (new, util/dirswap.go) replaces RemoveAll+Rename with rename-aside + rename-into-place + cleanup, so a concurrent unlocked reader sees either the old or the new tree, but never neither. A crashed swap is recoverable from the "aside" copy on the next sync. - Stale scratch directories left by a killed-mid-sync process (clone temp dirs, interrupted swap asides, and the legacy pre-lock naming scheme) are swept on a generous age threshold under the lock. - BuiltInSource/LocalSource/MemSource.Sync now correctly report "false" (no synchronisation performed) instead of "true": they were unconditionally poisoning Sources.isSynchronised, which made every later "sync and retry" elsewhere in the codebase a silent no-op. TestConcurrentSyncInProcess and TestConcurrentSyncAcrossProcesses from the previous commit now pass, along with new coverage for the swap recovery, stale-scratch sweep, and lock-timeout fallback paths.
High: syncGit's "git pull" fast path mutated finalDest's working tree in place with no lock held at the time it was added, and two concurrent pulls could also collide on .git/index.lock, escalating into a destructive re-clone via the "assume corrupted" fallback. Drop the pull path entirely; always clone to a fresh temp dir and swap it in, using "--reference-if-able --dissociate" against the existing clone so the network cost stays close to a pull's. Medium: log at Info level when acquireSyncLock waits more than a second, so lock contention is visible without needing -v/Trace. Low: resolve the lock path to absolute before using it as the process-local mutex key, so two callers that reach the same lock file via different relative paths still serialise against each other; remove the now-redundant swapDir wrapper and its duplicate test; document the acquire()/PID-write race window in util/flock now that it's load-bearing for lock re-entrancy; document syncedSince's fsTimeGranularity slack; correct doc comments that overclaimed either NewGitSourceWithLockTimeout's test-only-ness or SwapDir's rename gap being unobservable. Also replaces TestSyncLockTimeoutFallsBackToExistingCopy's fixed sleep with a ready-file handshake from the lock-holding child process (fixed sleeps are flaky under load) and guarantees that child is reaped via t.Cleanup even if an earlier assertion fails the test first.
jason-rl
force-pushed
the
jason/sync-race-04-adjacent-races
branch
from
July 27, 2026 20:47
2550450 to
d38079f
Compare
…tal fetch --reference-if-able (plus --dissociate) was meant to keep an already-synced source's re-sync cost close to a "git pull", by letting the new clone borrow objects from the existing one instead of re-fetching them. It never worked: finalDest is always itself a shallow (--depth=1) clone, and git unconditionally refuses to use a shallow repository as a reference/alternate, so the flag was silently a no-op and every sync paid for a full fresh clone anyway -- with no test covering the actual clone mechanism to catch it. Replace it with a local, working-tree-less clone of finalDest (same-filesystem, not a network operation) followed by a shallow fetch of just the latest commit from the real source and a checkout of that commit. Verified against the real default source (632 manifests): ~0.9s versus ~3.3s for a fresh clone, close to the ~0.7s a "git pull" on an already-current clone takes. Add a test exercising this incremental path against a real git binary, since none of the existing fakes simulate a second sync over an already-cloned finalDest.
jason-rl
force-pushed
the
jason/sync-race-04-adjacent-races
branch
from
July 27, 2026 21:46
d38079f to
0bd5b8a
Compare
…t syncs
Independent review caught that the previous commit's incremental path left
finalDest in a detached-HEAD state after "git checkout --detach FETCH_HEAD".
"git clone" only copies a source's "refs/heads/*", not a detached HEAD, so the
next incremental sync's local clone of finalDest had zero branches to offer as
"have"s during its own "git fetch --depth=1" -- silently degrading every sync
after the second into the same full-clone cost this path exists to avoid.
Verified empirically: with "checkout --detach", finalDest loses its last real
ref by the second incremental sync and its fetch negotiation falls back to a
full pack transfer; checking out onto a persistent local branch instead
("checkout -B") keeps every subsequent fetch negotiating a clean incremental
ACK, indefinitely.
Also make syncGit self-healing again for this path: if the incremental update
fails (eg. finalDest's ".git" is corrupt or truncated), fall back to a fresh
clone instead of surfacing the failure, restoring the same recovery behaviour
a from-scratch sync always had.
Rewrite the incremental-path test to use a "file://" source (a bare local path
silently ignores "--depth", which would hide exactly this class of bug),
repeat the sync several times to actually exercise the persistence issue above,
verify the persistent branch ref and an upstream deletion both propagate
correctly, and isolate it from the running machine's git config/hooks. Add a
second test covering the new corrupt-clone fallback.
jason-rl
force-pushed
the
jason/sync-race-04-adjacent-races
branch
from
July 27, 2026 22:14
0bd5b8a to
3c631dc
Compare
The doc comment explaining why detached HEAD was replaced with a named branch relied on "git clone --no-checkout" never writing a ".git/index", which is what actually makes "checkout -B" materialise the worktree. Add "--force" so this doesn't depend on that subtlety: without it, a checkout git considers a no-op would silently leave dest's worktree empty, discarding the manifest tree.
…nknown package Belt to the previous commit's braces, and worth it independently: machines will run mixed Hermit versions for a while, and an older binary sharing a state dir still syncs destructively without taking the new lock. sources.ErrSourceUnavailable is now reported (via a uriFS.dir field and Open override) when a source's entire backing directory is missing, as opposed to the directory existing but simply not containing the requested manifest. The distinction matters: a git source's directory can be transiently absent while another Hermit process is mid-sync, which is not evidence the package doesn't exist. uriFS.dir is left unset for in-memory sources (BuiltInSource/MemSource), since vfs.InMemoryFS unconditionally returns fs.ErrNotExist and would otherwise be misreported as unavailable on every lookup. manifest.Loader.get now keeps searching remaining bundles when one is unavailable rather than bailing out immediately, so one transiently- missing source never masks a package provided by another, healthy source, and only reports ErrSourceUnavailable if the package was found nowhere. Load retries on that specific error with a short bounded backoff (~620ms worst case) before falling back to its existing sync-and-retry, so a genuinely unknown package is never delayed by it. The ErrUnknownPackage message now also enumerates the sources that were searched, which previously gave no indication that a misconfigured or inaccessible source was the real cause. Also fixes errors.Wrap(err, err.Error()) in Load, which duplicated the wrapped error's message.
High: Load slept through the full sourceUnavailableRetryBackoff before ever calling Sync, so a source that has simply never been cloned paid ~620ms of pure latency every time before the sync that could actually fix it ran. Sync first, then only fall back to the bounded backoff for the remaining case: a sibling process's concurrent sync of this specific source completing while our own Sync call was a no-op. Medium: get() no longer caches a manifest found in a lower-preference bundle when a higher-preference bundle was unavailable at lookup time. Caching it would let a transient outage permanently invert source precedence for the rest of the process's lifetime; leaving it uncached lets the next lookup retry the unavailable bundle and self-heal once it recovers. TestLoaderFallsBackToHealthySourceWhenAnotherIsUnavailable still passes -- availability-over-precedence fallback still happens per-lookup, it just isn't permanently pinned. Low: document that uriFS.Open's directory-missing check is retrospective and best-effort, not an authoritative point-in-time answer -- it's only ever used as a retry signal.
…or fallback The three call sites that fall back to an alternate resolution strategy (a virtual package, a resync-then-retry, a glob-selector search) on manifest.ErrUnknownPackage predate sources.ErrSourceUnavailable, and didn't know about it: a source that's merely unreachable right now silently skipped the same fallback a genuinely-missing package would trigger, even though the alternate strategy may well succeed via a different, healthy source. Broaden all three checks to also match ErrSourceUnavailable.
Two more races on the same "hermit exec" hot path, independent of the git source sync issue fixed earlier in this stack. internal/dao.UpdatePackage wrote a package's cached etag with a plain os.WriteFile, which truncates the existing file before writing the new content. A concurrent GetPackage could observe a torn (empty or partial) etag -- not merely cosmetic, since 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 exec'ing. UpdatePackage now writes to a temp file in the same directory and renames it into place, so a reader always sees either the old, complete value or the new one. UpdateCheckedAt is now also stored explicitly instead of inferred from the file's mtime, which a torn write could also disturb; a legacy (pre-JSON-envelope) metadata file written by an older Hermit version still falls back to mtime. state.linkBinaries built its symlink directory with RemoveAll + recreate, which CacheAndUnpack's unlocked pre-lock fast path (areBinariesLinked) can observe mid-rebuild: a caller that sees "already linked" may go on to exec a binary through a directory that gets removed out from under it moments later. It now builds the new set of symlinks in a temporary sibling directory and swaps it into place with util.SwapDir (introduced earlier in this stack for the git source fix), so readers only ever see the complete old or new set.
… 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.
- state.extract: archive.Extract's own deferred rename already publishes p.Dest before EventUnpack's trigger runs, so cleaning it up on trigger failure with plain os.RemoveAll is the same reader-unsafe unlink-storm this stack replaced everywhere else. Use util.RemoveAllAtomic instead, consistent with removeRecursive's existing use of it. - util.AtomicWriteFile: fsync the temp file before renaming it into place. Without this, a crash shortly after a successful-looking write can still leave the renamed-to path pointing at a zero-length or truncated file, since the rename being durable doesn't make the data behind it durable. - util.RemoveAllAtomic: document that, unlike os.RemoveAll, it requires dir's parent to exist (it needs somewhere to create the sibling via MkdirTemp). - internal/dao.UpdatePackage: document that the etag and checked-at sidecar are written via separate renames, so two concurrent UpdatePackage calls for the same package can interleave and pair a stale etag with a fresh checked-at time. Carried over from the single-JSON-file format this replaced, not introduced by the two-file split.
…accuracy - 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.
…review util.AtomicWriteFile's fsync-omission rationale claimed internal/dao.UpdatePackage runs on Hermit's "exec" hot path -- it doesn't; it's gated behind an update-interval check and always follows a network round trip, so the fsync cost was never actually avoided on every invocation. The comment also ignored the helper's other caller, Env.SetEnv/DelEnv, which rewrites the user's bin/hermit.hcl and is the caller for which losing data to a crash is actually consequential. dao.UpdatePackage's comment on its etag/checked-at interleave risk described it as carried over from "the single-JSON-file format this replaced" -- that JSON format never existed on master; it was introduced and removed again within this same PR stack. The real predecessor (a single etag file whose mtime doubled as the checked-at time) still supports the same conclusion -- the mismatched pairing is newly possible with the two-file split -- just not for the reason originally given. Also tightens the state.go comment on the p.Files copy-loop cleanup: the previous wording described a "wedged" retry, but for the common case where a manifest doesn't override "root", a leftover p.Dest instead makes isExtracted see it as already-installed and skip re-extraction on retry entirely, silently omitting the copied files forever -- worse than "wedged", and worth saying so.
jason-rl
force-pushed
the
jason/sync-race-04-adjacent-races
branch
from
July 27, 2026 22:41
3c631dc to
de3bb66
Compare
jason-rl
marked this pull request as ready for review
July 27, 2026 22:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #591 (3/4) -- this PR's diff includes all prior commits; please review via the Commits tab (only the last five commits, "internal/dao, state: atomic writes for two more exec-path races", "internal/dao, state, env: drop JSON envelope, add shared atomic-write helper", "state, internal/dao, util: durability fixes and doc caveats from review", "state, internal/dao, util: revert hot-path fsync, fix leak and doc inaccuracy", and "internal/dao, state, util: correct two doc-comment inaccuracies from review", are new here).
Two more races on the same "hermit exec" hot path, found while investigating the git source sync race fixed in #589-#591. Independent of it -- these are non-atomic writes to on-disk state that a concurrent invocation can observe torn or momentarily missing.
internal/dao.UpdatePackagewrote a package's cached etag with a plainos.WriteFile, which truncates the existing file before writing the new content. A concurrentGetPackagecould observe a torn (empty or partial) etag -- not merely cosmetic, sinceUpgradeChanneltreats any etag change, including a corrupted one, as a reason toevictPackage(rm -rf) a package tree that another process may be actively exec'ing. Both the etag and its check timestamp are now written atomically (temp file in the same directory, renamed into place) via a new sharedutil.AtomicWriteFilehelper, also used byenv.go'sSetEnv/DelEnv..checkedsidecar file, with a graceful fallback to the etag file's mtime when that sidecar is missing or unparseable -- the same degraded-but-safe behaviour as running against an older Hermit that never writes it at all.DAO.Opennow sweeps stale.tmp-*scratch files (left behind when a process is killed before its deferred cleanup can run) out of the metadata directory, bounded by a generous age threshold so a genuinely in-flight write from another process is never touched.state.linkBinariesbuilt its symlink directory withRemoveAll+ recreate, whichCacheAndUnpack's unlocked pre-lock fast path (areBinariesLinked) can observe mid-rebuild: a caller that sees "already linked" may go on to exec a binary through a directory that gets removed out from under it moments later. It now builds the new set of symlinks in a temporary sibling directory and swaps it into place withutil.SwapDir(introduced in sources: fix intermittent "unknown package" errors from concurrent, unlocked source syncs (2/4) #590 for the git source fix).state.removeRecursive's final cleanup similarly now uses a newutil.RemoveAllAtomic(rename-aside, then remove) instead of a plainRemoveAll, closing the same reader-visible window for removal thatSwapDircloses for replacement.WritePackageStatestoring a zeroUpdateCheckedAtas "now" (a pre-existing behaviour ofdao.UpdatePackage) is now documented as harmless specifically becauseEnsureChannelIsUpToDateshort-circuits onUpdateInterval == 0before ever consulting it.state.extractcleaned up a failedEventUnpacktrigger with a plainos.RemoveAll(p.Dest), butarchive.Extracthas already renamedp.Destinto its final, real location by the time that trigger runs -- so this was the same reader-unsafe unlink-storm this PR replaces everywhere else, just on the error path. Now usesutil.RemoveAllAtomic, consistent withremoveRecursive. The identical gap in the adjacentp.Filescopy loop, which runs after the samearchive.Extractcall but before the trigger, was found on a later review pass and fixed the same way.util.AtomicWriteFilebrieflyfsync'd the temp file before renaming it into place, to close a narrow crash-durability gap (a crash between a successful-looking write and the data actually reaching disk could otherwise leave the renamed-to path pointing at a zero-length or truncated file). That was reverted after benchmarking: on macOS, Go'sFile.Syncissuesfcntl(F_FULLFSYNC), measured here at ~140x slower than a plain write (~4.3ms vs. ~30µs/op). The doc comment justifying the revert originally claimed this cost was paid on everyhermit execviainternal/dao.UpdatePackage-- caught by review as wrong: that call is gated behind an update-interval check and always follows a network round trip, so the fsync was never actually on a hot path. The comment now states the real tradeoff:UpdatePackage's writes (a cached etag and check timestamp) aren't authoritative, so losing one to a crash just costs one extra upstream check, while the helper's other caller,Env.SetEnv/DelEnvrewriting the user'sbin/hermit.hcl, is where losing data to a crash is actually consequential -- accepted here as the cost of one shared helper rather than special-casing fsync per caller.util.RemoveAllAtomicnow notes it requiresdir's parent to exist (unlikeos.RemoveAll, which is nil-safe for a fully-missing path).dao.UpdatePackage's note on its etag/.checkedinterleave risk was corrected twice: a first pass said the risk was "carried over" from the single-JSON-file format this PR replaced, but that format wrote both fields in one atomic rename, so the interleave is actually newly possible with the two-file split; a second review pass then caught that the JSON format itself never existed onmaster-- it was introduced and removed again within this same PR stack -- so the comment now describes the real predecessor (a single etag file whose mtime doubled as the checked-at time), which still supports the same conclusion. Thestate.extractcomment on thep.Filescopy-loop cleanup was also reworded: a leftoverp.Destthere doesn't just leave a retry "wedged" as originally described -- for the common case where a manifest doesn't overrideroot, it makesisExtractedsee the package as already installed and skip re-extraction on retry entirely, silently omitting the copied files forever.Test plan
go build ./...go test ./... -race -count=1(full suite)golangci-lint run ./...(full repo)This PR -- the investigation, code, and tests -- was drafted with AI assistance (Claude Code).