sources: fix intermittent "unknown package" errors from concurrent, unlocked source syncs (2/4) - #590
Open
jason-rl wants to merge 7 commits into
Open
sources: fix intermittent "unknown package" errors from concurrent, unlocked source syncs (2/4)#590jason-rl wants to merge 7 commits into
jason-rl wants to merge 7 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.
This was referenced Jul 27, 2026
Open
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-02-lock-fix
branch
from
July 27, 2026 20:47
a8f1453 to
b4fb272
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.
…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.
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.
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 #589 (1/4) -- this PR's diff includes both of that PR's commits too; please review via the Commits tab (only the last five commits, "sources: serialise and de-destruct git source syncing", "sources: remove pull-path race, harden lock, address review nits", "sources: replace inert --reference-if-able clone with a real incremental fetch", "sources: keep the incremental clone's local branch alive across repeat syncs", and "sources: make incremental checkout robust to a non-empty index", are new here).
This is the actual fix for the race reproduced in #589: running a not-yet-installed binary several times within a few milliseconds of each other can intermittently fail with
unknown package, because concurrent syncs of the same manifest source raced and destroyed each other's output mid-replace.sources/lock.goaddsacquireSyncLock: a cross-processflockplus a process-localsync.Mutex(needed becauseutil/flockis deliberately re-entrant per-PID, so it's a no-op between goroutines of the same process). The lock path is resolved to absolute before use, and acquisitions that wait more than a second are logged at Info level so contention is visible without needing-v/Trace.GitSource.Syncnow 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.git pullfast path. It mutated the target's working tree directly with no lock held, and two concurrent pulls could also collide on.git/index.lock, which the "assume corrupted" fallback would escalate into a destructive re-clone. Every sync now clones to a fresh temp dir and swaps it in.--reference-if-able --dissociateagainst the existing clone (when present), meant to keep the network cost close to a pull's. That never worked: the existing clone is always itself shallow (--depth=1), and git unconditionally refuses to use a shallow repo as a reference, so the flag was silently a no-op and every sync paid for a full fresh clone -- caught by review, since no test exercised the actual clone mechanism. It's now a local, working-tree-less clone of the existing copy followed by a shallow fetch of just the latest commit and a checkout of that commit, which really does keep the cost close to a pull's (verified against the real default source: ~0.9s vs. ~3.3s for a fresh clone, vs. ~0.7s for a plaingit pull), plus a new test that drives this path against a realgitbinary.--reference-if-ablefix above was meant to solve, just one layer down: a baregit cloneonly copies a source'srefs/heads/*, not a detached HEAD, so the next incremental sync's local clone of it had zero branches to offer as ahavewhen fetching from the real source -- degrading every sync after the first back into a full pack transfer, and (verified empirically over repeated syncs) losing the branch entirely by the second incremental sync. Also caught by review, since the original test only exercised a single sync. Fixed by checking out onto a persistent local branch (git checkout -B hermit FETCH_HEAD) instead of detaching, so the branch ref survives into each subsequent local clone; the test now drives four sequential incremental syncs (including a file-deletion propagation check) and asserts the branch stays checked out throughout, over afile://transport (a bare local path silently ignores--depth, which would have masked this).finalDest's.gitis corrupt or truncated) instead of surfacing the failure, matching the self-healing behaviour a from-scratch sync always had -- with a new test pre-creating a corrupt.gitto exercise it.git checkout -B hermit FETCH_HEADthat materialises the incremental worktree only does so becausegit clone --no-checkoutwrites no.git/index, which is what makes git treat it as an initial checkout rather than a no-op. Caught by review as a subtlety worth not depending on: added--forceso the worktree is populated unconditionally, regardless of that implementation detail.util.SwapDir(new) replacesRemoveAll+Renamewith rename-aside + rename-into-place + cleanup, so a concurrent unlocked reader sees either the old or the new tree, but never neither. Within oneSwapDircall, a failed second rename restores the previous tree immediately. A crash strictly between the two renames (eg. SIGKILL) is not recovered from automatically -- the target is simply missing until the next sync completes successfully, at which point the stale "aside" copy is deleted (not promoted back) as part of that sync's own cleanup.BuiltInSource/LocalSource/MemSource.Syncnow correctly report "false" (no synchronisation performed) instead of "true": they were unconditionally poisoningSources.isSynchronised, which made every later "sync and retry" elsewhere in the codebase a silent no-op.#589's
TestConcurrentSyncInProcess/TestConcurrentSyncAcrossProcessesnow pass, along with new coverage for the swap recovery, stale-scratch sweep, lock-timeout fallback, and (new) incremental-clone paths. The lock-timeout fallback test now synchronises on the lock-holding child process actually confirming it holds the lock (via a ready file) instead of a fixed sleep, and reliably reaps that child even if an earlier assertion fails the test.TestSyncGitIncrementalUpdate(new) drives the incremental-update branch against a realgitbinary rather than a fake runner, since that's what actually caught the--reference-if-ableregression above; it's since been extended to drive four sequential syncs against an isolated git environment (HOME/XDG_CONFIG_HOME/GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEMall pointed at throwaway paths) overfile://, which is what caught the detached-HEAD regression above.TestSyncGitIncrementalUpdateFallsBackOnCorruptClone(new) covers the fresh-clone fallback.Test plan
go build ./...go test ./sources/... ./util/... -race -count=1 -v(including sources: reproduce intermittent "unknown package" errors under concurrent execs (1/4) #589's reproducer, now passing)golangci-lint run ./sources/... ./util/...This PR -- the investigation, code, and tests -- was drafted with AI assistance (Claude Code).