Fix cache bind mounts - #42
Conversation
A daemon whose filesystem view is separate from the host process's -
Docker Desktop's VM being the reported case - can see a freshly
recreated cache tree as missing, so the strict --mount form rejected
the package-cache binds ("bind source path does not exist" under
/host_mnt) after deleting ~/Library/Caches/org.eclipse.enclave on
macOS.
Mark disposable directory binds (package caches, image inbox) for source
creation and render them with the --volume host-path form - still plain
bind mounts, no managed volumes - while required project, config, auth,
and file mounts stay strict. Package-cache sources are created host-side
before use and creation failures now propagate instead of being ignored.
The auth-reconcile helper inlines the script content read host-side
rather than bind-mounting it from the extracted asset cache, gateway
image build output is surfaced instead of discarded, and the node-dev
entrypoint no longer aborts the session when the nvm version cache is
unwritable.
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Snapshot the reconcile script when the Docker backend is initialized so post-session credential sync no longer reads from a disposable extracted asset tree. This keeps cache deletion functionally safe for long-running sessions. Add regression coverage and document the backend-lifetime behavior. Signed-off-by: Kathrin Hanauer <kal@xaikal.org>
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Fixes #41 by making the disposable package caches survive a deleted cache root. backend.Mount gains CreateSourceDir, which the Docker backend renders as --volume src:dst instead of the strict --mount type=bind (the only CLI form that creates a missing source), and the QEMU backend handles with an explicit MkdirAll. Cache sources are now created host-side in a loop driven by packageCacheDirs, the image inbox is marked disposable too, the auth reconcile script is snapshotted into memory at backend construction instead of being bind-mounted from the extracted asset cache, and the node-dev entrypoint tolerates an unwritable nvm version cache.
The approach looks sound and the tests cover the interesting bits. I verified the claim in docs/persistence.md that no required runtime mount comes from the cache tree: skills, gateway config, TLS and history/memory mounts all resolve under the state root, so the scoping holds.
Main points for a maintainer:
addCacheMountsturning aMkdirAllfailure into a hard session-start error is the one place the code contradicts the invariant the PR is establishing, and it diverges from the warn-and-skip style of the neighbouring mount helpers.- The
--volumestrictness argument in theCreateSourceDirdoc comment does not hold on SELinux-enforcing hosts, where every bind is already rendered as--volume. - The gateway build output change is unrelated to the cache fix and rides in the same commit; it also ignores
--progressand now prints a full failed build log on the recovered retry path.
| for _, entry := range packageCacheDirs(r.containerHome) { | ||
| source := filepath.Join(cacheDir, entry[0]) | ||
| if err := os.MkdirAll(source, 0o700); err != nil { | ||
| return fmt.Errorf("create package cache directory %s: %w", source, err) |
There was a problem hiding this comment.
A failed MkdirAll here now aborts the whole session start, which works against the invariant this PR establishes (losing cache data must never block a session) and against the sibling mount helpers, which warn and skip: addImageInboxMount. The mount is marked CreateSourceDir anyway, so warning and skipping that one cache keeps a full or read-only cache filesystem from taking the run down with it.
| func (r *Runtime) addCacheMounts(mounts *mountAccumulator) { | ||
| // packageCacheDirs maps each package-cache directory under the per-project | ||
| // cache root to its in-container mount point (relative to the container home). | ||
| func packageCacheDirs(containerHome string) [][2]string { |
There was a problem hiding this comment.
[][2]string with entry[0]/entry[1] at the call site reads worse than the named-field table used for exactly this a few lines further down (addToolConfigMounts). A []struct{ cacheName, containerPath string } would match it.
| // cache data whose loss must never prevent a session from starting. Backends | ||
| // create a missing source as an empty directory instead of failing. Required | ||
| // project, config, auth, and file mounts must not set it, so their missing | ||
| // sources stay hard errors. |
There was a problem hiding this comment.
The "missing sources stay hard errors" guarantee only holds when SELinux is not enforcing. applySELinuxMounts moves every bind mount into Binds (formatBind), and buildRunArgs renders those as --volume (here), so on those hosts required mounts are already source-creating. The wording should be scoped, or the strictness made explicit for required mounts.
| // Build output goes to the terminal like the runtime image build, so a | ||
| // gateway build failure is attributable instead of surfacing later as an | ||
| // unrelated container-start error. | ||
| if err := docker.Build(context.Background(), req, os.Stdout); err != nil { |
There was a problem hiding this comment.
The gateway BuildRequest never sets Progress, so --progress quiet is honored for the runtime image build (build.go) but silently ignored here now that the output is no longer discarded.
| // network for Alpine index fetches. Retry once with host build network. | ||
| req.NetworkMode = "host" | ||
| if retryErr := docker.Build(context.Background(), req, io.Discard); retryErr != nil { | ||
| if retryErr := docker.Build(context.Background(), req, os.Stdout); retryErr != nil { |
There was a problem hiding this comment.
On the common BuildKit-DNS path the user now gets a full failing build log followed by a successful one, with only a warning at the end explaining it. Buffering the first attempt and flushing it only if the retry also fails keeps the recovered case quiet without losing attribution for real failures.
|
Related: I noticed that if someone uses the |
3 participants
What it does
Fixes issue #41
How to test
See issue #41
Follow-ups
Breaking changes
Review checklist