cashapp / cashapp/hermit

Races on the "hermit exec" hot path: intermittent "unknown package" (and two related failures)

Open
#593 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
926
Forks
111
Avg merge
7m
Merged PRs (30d)
1

Description

## Summary

Running 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:

```
fatal:hermit: : unknown package
```

even though the package is perfectly valid.

Investigating this surfaced four related races on the same "hermit exec" hot path. **Races A and B are the confirmed, direct cause of the `unknown package` message above** -- both go through `manifest/loader.go`'s `ErrUnknownPackage` path. **Races C and D are separate, adjacent races found during the investigation**: they don't produce `unknown package` -- they surface (when they surface at all) as a different fatal error, `": failed to execute \"\": no such file or directory"` (`env.go`'s `syscall.Exec` wrapper) -- but they're real correctness bugs on the same "thing you just ran may not actually be ready yet" hot path, so they're included and fixed here too.

All four are reproduced below directly against unpatched `master`, with captured output, and fixed as four independently-reviewable, stacked PRs.

---

## Race A -- concurrent `GitSource.Sync` destroys the manifest tree a reader is using

*Produces the reported `unknown package` error.*

`sources/git.go`'s `Sync`/`syncGit` has no cross-process or cross-goroutine locking. N processes/goroutines syncing the same not-yet-cloned source all clone independently, and each then wipes and replaces the shared manifest tree with `os.RemoveAll(finalDest)` + `os.Rename(dest, finalDest)`. A reader using `os.DirFS(finalDest)` concurrently can observe `ENOENT` mid-replace, which `manifest/loader.go`'s `load()` treats as "manifest not found" -- indistinguishable, at that point, from a package that never existed -- and reports as `ErrUnknownPackage`, i.e. exactly the error text above.

**Steps to reproduce:** 8 processes (or goroutines) call `GitSource.Sync` concurrently against the same not-yet-cloned source directory, while a reader repeatedly reads a manifest file from that directory.

**Expected:** a manifest a reader has already successfully found should never subsequently vanish.

**Actual** (captured against `master`):
```
=== RUN TestConcurrentSyncInProcess
git_concurrency_test.go:171: Did not expect an error but got:
manifest disappeared after first appearing: .../2ea20b82.../pkg.hcl
--- FAIL: TestConcurrentSyncInProcess (0.06s)
=== RUN TestConcurrentSyncAcrossProcesses
git_concurrency_test.go:224: Did not expect an error but got:
manifest disappeared after first appearing: .../7c2cdea7.../pkg.hcl
--- FAIL: TestConcurrentSyncAcrossProcesses (1.14s)
```
Reproduced both with goroutines sharing a process and with genuine separate child processes.

Fixed by #589 (regression test) + #590 (fix).

---

## Race B -- a transiently-missing source directory is misreported as a permanently unknown package

*Also produces the reported `unknown package` error -- this is the loader-side companion to Race A, widening coverage to mixed-Hermit-version fleets and any residual window in the fix for Race A (e.g. an older Hermit binary sharing a state dir, which doesn't take the lock added for Race A).*

`manifest/loader.go`'s `load()` maps *any* `os.ErrNotExist` from a source -- whether just the single requested manifest file is missing, or the entire backing directory is transiently absent -- to "not found," which `get()` turns into `ErrUnknownPackage`. A package that genuinely exists in a healthy source is indistinguishable from one that never existed, if a resync (by any Hermit version) happens to catch it mid-swap.

**Steps to reproduce:** populate a real package manifest via a git source, confirm `Loader` resolves it, then concurrently repeat master's actual swap sequence (`os.RemoveAll` + recreate) against tight-loop `Loader.get` calls for that same, real, already-resolving package.

**Expected:** a package that exists in a healthy source should never be reported as unknown merely because another process is mid-resync.

**Actual** (captured against `master`):
```
observed foo (a real, existing package) reported as: foo: unknown package
```

Fixed by #591.

---

## Race C -- `internal/dao.UpdatePackage`'s non-atomic write yields a torn etag read

*Does NOT produce "unknown package". If it surfaces at all, it's as `": failed to execute \"\": no such file or directory"` (`env.go:912-913`), because a torn etag read can make `UpgradeChannel` wrongly `evictPackage` (`rm -rf`) a package tree that another process is concurrently trying to exec. It can also be entirely silent (an unnecessary re-download/re-check) if no exec races the eviction.*

`UpdatePackage` writes a package's cached etag with a plain `os.WriteFile`, which truncates the existing file before writing new content. A concurrent `GetPackage` can observe the file mid-write.

**Steps to reproduce:** one goroutine calls `UpdatePackage` in a loop with real-sized etag content, another concurrently calls `GetPackage` in a loop, asserting the returned etag is always a value that was actually, fully written.

**Expected:** a concurrent read of a package's cached etag should always see a complete, valid value (old or new), never a torn one.

**Actual** (captured against `master`): 30/30 runs (`-count=30 -race`) failed with:
```
observed torn/unexpected etag: ""
```

Fixed by #592.

---

## Race D -- `state.linkBinaries`'s destructive rebuild races `areBinariesLinked`'s unlocked pre-check

*Does NOT produce "unknown package". Same failure mode as Race C if it surfaces: `": failed to execute \"\": no such file or directory"` from `env.go:912-913`, since `CacheAndUnpack` may go on to exec a binary through a directory it only just observed (racily) as "already linked".*

`linkBinaries` builds its symlink directory with `os.RemoveAll` + recreate. `CacheAndUnpack`'s unlocked pre-lock fast path (`areBinariesLinked`) can observe "already linked" and then find the directory removed out from under it moments later, because another goroutine/process's `linkBinaries` call is mid-rebuild.

**Steps to reproduce:** run `linkBinaries` in a tight loop concurrently with a loop that calls `areBinariesLinked()` immediately followed by re-statting the link.

**Expected:** once the pre-lock check reports binaries as linked, that should remain true long enough for the caller to actually use them.

**Actual** (captured against `master`): 875 instances, in a 2-second window, of `areBinariesLinked() == true` immediately followed by a failed stat.

Fixed by #592.

---

## Fixes

| Race | Produces reported `unknown package`? | PR |
|---|---|---|
| A | Yes | #589 (repro), #590 (fix) |
| B | Yes | #591 |
| C | No -- different error / silent | #592 |
| D | No -- different error | #592 |

All four PRs are stacked (each explains, in its description, which single commit is new relative to the previous one) and are up for independent review.

---
*This issue -- including the investigation and reproductions above -- was drafted with AI assistance (Claude Code).*

Contributor guide

No contributing guide indexed for this repository

Research direction

The investigation covers sources/git.go, manifest/loader.go, internal/dao.UpdatePackage, state.linkBinaries, and env.go, with reproduction coverage in git_concurrency_test.go. Start by reading the linked PRs and their regression tests; the work is complete when the four reported races no longer produce disappearing manifests, torn etags, or missing binaries under concurrent execution.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
cli, tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.