sources, manifest: don't misreport a transiently-unavailable source as "unknown package" (3/4) - #591
Open
jason-rl wants to merge 10 commits into
Open
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.
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-03-reader-resilience
branch
from
July 27, 2026 20:47
2811bad to
4fff3cd
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-03-reader-resilience
branch
from
July 27, 2026 21:46
4fff3cd to
0c5b056
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-03-reader-resilience
branch
from
July 27, 2026 22:14
0c5b056 to
ba2e728
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.
jason-rl
force-pushed
the
jason/sync-race-03-reader-resilience
branch
from
July 27, 2026 22:41
ba2e728 to
6929282
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 #590 (2/4) -- this PR's diff includes all prior commits; please review via the Commits tab (only the last three commits, "sources, manifest: distinguish a transiently-missing source from an unknown package", "manifest, sources: fix retry ordering, avoid caching a shadowed manifest", and "env: treat a transiently-unavailable source like an unknown package for fallback", are new here).
Belt to #590'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 -- so a reader can still momentarily see a source's directory vanish. Before this PR, that transient condition and a genuinely nonexistent package look identical to the caller: both surface as
unknown package.sources.ErrSourceUnavailableis now reported (via auriFS.dirfield andOpenoverride) 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.diris left unset for in-memory sources, sincevfs.InMemoryFSunconditionally returnsfs.ErrNotExistand would otherwise be misreported as unavailable on every lookup. (This check is necessarily a best-effort, retrospective heuristic -- it's only ever used as a retry signal, never an authoritative answer.)manifest.Loader.getnow 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 reportsErrSourceUnavailableif the package was found nowhere. It also no longer caches a manifest found in a lower-preference bundle while a higher-preference one 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.Loadretries onErrSourceUnavailablewith a short bounded backoff (~620ms worst case), but only after actively syncing the source first -- a source that's simply never been cloned needs a real sync to ever become available, so sleeping through the backoff first would add up to ~620ms of pure latency to every such cold start for nothing. A genuinely unknown package is never delayed by any of this.ErrUnknownPackagemessage now also enumerates the sources that were searched.errors.Wrap(err, err.Error())inLoad, which duplicated the wrapped error's message.env.go's three call sites that fall back to an alternate resolution strategy (a virtual package, a resync-then-retry, a glob-selector search) onmanifest.ErrUnknownPackagepredateErrSourceUnavailable, and didn't know about it, so they silently skipped that same fallback when a source was merely transiently unreachable -- even though the alternate strategy may well succeed via a different, healthy source. All three now also matchErrSourceUnavailable.Test plan
go build ./...go test ./sources/... ./manifest/... -race -count=1golangci-lint run ./sources/... ./manifest/...This PR -- the investigation, code, and tests -- was drafted with AI assistance (Claude Code).