Skip to content

fix: exclude Docker nested bind-mount points from git clean during source sync#46

Merged
pilat merged 1 commit into
mainfrom
pilat/update-too-short
Jul 3, 2026
Merged

fix: exclude Docker nested bind-mount points from git clean during source sync#46
pilat merged 1 commit into
mainfrom
pilat/update-too-short

Conversation

@pilat

@pilat pilat commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Why

devbox update (and devbox up) fail while the project is already running. Source sync runs git reset --hard + git clean in each source checkout, and git clean tries to rmdir a directory that Docker is actively using as a nested bind-mount point — which returns Permission denied:

failed to update sources: failed to sync source: failed to reset repo
.../sources/service-a: failed to clean:
warning: failed to remove cmd/service-a/shared/: Permission denied
 exit status 1

The failure cascades: the first source to fail cancels the shared context, so other in-flight sources abort too. Today the only workaround is downupdateup.

The trigger is a compose service with two bind volumes where one target is nested under the other, e.g. ./sources/service-a/cmd/service-a → /app and ./sources/shared → /app/shared. Because /app is the host source dir, Docker materializes the mountpoint cmd/service-a/shared on the host and mounts a different source there. While the stack is up, that host dir is a live mount git clean cannot remove.

Note this is not the -fdx change from #40: the mountpoint is untracked and not gitignored, so plain git clean -fd hits it too. Dropping -x would not help — a path-based exclude is the only thing that works.

What

Compute the set of nested mountpoint paths from the compose volume topology and pass them to git clean as -e <path> excludes, per source.

The discriminator is deliberately narrow: exclude a nested bind mount only when its own host source is a different source under sources/ than the enclosing parent mount's source. That isolates the one genuine case and leaves the lookalikes alone:

  • Self-nested same-source mounts (a service's own cmd/<name> subdir, etc.) materialize as tracked dirs of the same source — git clean never rmdirs those, and excluding them would wrongly stop it from cleaning cruft inside.
  • File mounts (envs/<svc>/.env → /app/.env) materialize as file mountpoints, which git clean removes fine.

With no excludes the git clean invocation is byte-for-byte unchanged, so nothing else is weakened. git.New becomes variadic to carry the excludes, which keeps the git.Service interface (and its generated mocks) untouched.

Testing

  • Unit tests for the topology computation (positive case + self-nested / file-mount / locally-mounted-parent negatives, plus sort/dedup determinism since services come from a map).
  • Mock-level tests pinning the exact git clean … -e … args for both -fd and -fdx.
  • A real-git test proving the computed exclude string actually anchors: the mountpoint survives clean -fdx -e <path> while sibling untracked cruft is removed.
  • An e2e regression test that stands up a stack with a live cross-source nested mount and runs update while it's up. Verified it fails without this fix (Permission denied, exit 1) and passes with it; the full e2e suite stays green.

Out of scope

Whether update should run git reset --hard + clean on sources that are actively mounted into a running stack at all (it currently wipes uncommitted changes there). Also, a non-sources/ bind (e.g. a locally-mounted source) nested as a directory into a checkout would still hit the same rmdir failure but isn't excluded — distinguishing that purely would need a runtime stat. Neither pattern is the crash being fixed here.

@pilat pilat self-assigned this Jul 3, 2026
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This PR computes per-source git-clean exclude patterns based on nested Docker bind-mount topology and threads them through the sync pipeline. git.New now accepts variadic excludes stored on the service, applied as -e flags during git clean in the reset phase.

Changes

Nested Mount Exclude Support

Layer / File(s) Summary
Compute nested mount excludes
internal/project/mounts_exclude.go
New SourceCleanExcludes method, nestedForeignMountExcludes helper, and sourceSegment helper derive repo-relative exclude paths for sources with nested bind mounts under other sources.
git.New accepts excludes and applies to clean
internal/git/git.go
New signature adds variadic excludes ...string, stored on svc; reset phase builds cleanArgs and appends -e <pattern> for each exclude during git clean.
Wire excludes into sync flow
cmd/devbox/update.go
syncSources computes excludes via p.SourceCleanExcludes() and passes excludes[name]... into each source's git.New call.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
    participant Update as syncSources
    participant Proj as Project
    participant Git as git.Service
    participant CLI as git CLI

    Update->>Proj: SourceCleanExcludes()
    Proj-->>Update: map[source][]excludePaths
    loop per source
        Update->>Git: New(repoDir, excludes[name]...)
        Git-->>Update: Service (excludes stored)
        Update->>Git: Sync/reset
        Git->>CLI: git -C targetPath clean <flags> -e pattern...
        CLI-->>Git: clean result
    end
Loading

Possibly related PRs

  • pilat/devbox#17: Both PRs modify per-source git client creation in cmd/devbox/update.go and git.New construction in internal/git/git.go.
  • pilat/devbox#40: Both PRs modify the git clean invocation within the sync/reset cleanup path in internal/git/git.go.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: excluding nested Docker bind-mount points from git clean during source sync.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pilat/update-too-short

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/project/mounts_exclude.go`:
- Around line 44-80: The nested bind handling in mounts_exclude.go is
incorrectly treating file bind mounts under a source mount as clean excludes,
which can preserve mounted files during reset/clean. Update the logic around the
bind scan in the loop over binds so it skips child mounts that are file mounts
before computing parent/child relationships and appending to result. Use the
existing sourceSegment, bindVol, and the parent/child target matching logic to
ensure only directory-backed source mounts contribute excludes, not nested file
binds like config.yaml.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6876022e-26e9-4237-ae05-e85f30525309

📥 Commits

Reviewing files that changed from the base of the PR and between cf9b69a and 29ee675.

⛔ Files ignored due to path filters (4)
  • internal/git/git_test.go is excluded by !**/*_test.go
  • internal/project/mounts_exclude_test.go is excluded by !**/*_test.go
  • tests/e2e/e2e_test.go is excluded by !**/*_test.go
  • tests/e2e/fixtures/manifest-nested/docker-compose.yml is excluded by !tests/e2e/fixtures/**
📒 Files selected for processing (3)
  • cmd/devbox/update.go
  • internal/git/git.go
  • internal/project/mounts_exclude.go

Comment thread internal/project/mounts_exclude.go
@pilat
pilat force-pushed the pilat/update-too-short branch from 29ee675 to ec0c9c7 Compare July 3, 2026 01:31
@pilat
pilat merged commit e6e62ca into main Jul 3, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant