fix(delivery): cleanup build files for mounted volume#615
Merged
Conversation
The workspace volume is mounted at the workspace folder, so its root is the git working tree. Two seed artifacts leaked into it: - The .devsy-seeded sentinel was written at the volume root, showing up as an untracked file. Track seeded state with an sh.devsy.seeded volume label instead (set at creation; the volume is removed on copy failure so the label never persists for an empty volume). - The cp -a copy pulled in devsy's transient .devsy-internal build folder. Switch to a tar copy that excludes it (preserving attributes/perms). Verified on ws3-ssh: the seeded workspace tree now contains only project content (no .devsy-seeded, no .devsy-internal), perms/.git preserved, and the label-based idempotency still skips re-seeding on subsequent ups.
✅ Deploy Preview for devsydev canceled.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
✅ Deploy Preview for images-devsy-sh ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…up eagerly Make the handling of devsy's build scaffolding (.devsy-internal) explicit and robust instead of implicit and scattered: - Add a single source of truth in pkg/devcontainer/config/artifacts.go: buildArtifactNames plus BuildArtifactExcludes() and RemoveBuildArtifacts(). Adding a future artifact now only requires updating that list. - Route every consumer through it: build/prebuild cleanup, the prebuild-hash exclude, and the workspace-volume seed tar-exclude no longer hard-code the folder name. - Fix the root cause of the seed-pollution bug: build() now removes the artifacts eagerly on the success path, so they are gone before any later step that copies the workspace tree. The deferred cleanup in Up/Build remains as an error-path safety net, and the seed exclude as defense in depth. Verified on ws3-ssh: a Dockerfile-based workspace seeds a clean tree (no .devsy-internal, no .devsy-seeded), .git and perms preserved, seeded label set.
Follow-up to the artifact centralization: reduce the verbose doc/inline comments to short why-notes now that the helpers and flow are self-documenting. No behavior change.
skevetter
changed the title
Dockerless (kubernetes) builds defer the actual image build to inside the container: build() prepares .devsy-internal on the host, stashes it, and kaniko consumes it later. The eager cleanup added for volume-seed hygiene deleted those artifacts prematurely, breaking dockerless builds with 'rename dir: .devsy-internal: no such file or directory'. Only clean up eagerly when the build produced an image (buildInfo.Dockerless == nil); the dockerless flow manages its own artifact lifecycle. Volume seeding never runs on the dockerless/kubernetes path, so tree pollution is not a concern there. Verified on an orbstack kubernetes cluster: dockerless up now succeeds and the pod runs.
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
Replace the timing-dependent eager RemoveBuildArtifacts in build() (which needed a dockerless carve-out to avoid deleting .devsy-internal before kaniko's second build phase) with the exclude-at-copy-time guarantee. The correctness invariant is now that any operation copying, streaming, or hashing a workspace tree excludes build artifacts via BuildArtifactExcludes; RemoveBuildArtifacts is best-effort tidiness only. Add the exclude to the StreamWorkspace and StreamMount tunnel paths, which previously honored only .devsyignore.
skevetter
force-pushed
the
fix/seed-workspace-tree-pollution
branch
from
July 7, 2026 18:23
975e1de to
a7b1134
Compare
skevetter
marked this pull request as ready for review
July 7, 2026 19:20
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/devcontainer/config/artifacts.go (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup errors are silently discarded.
RemoveBuildArtifactsswallows allos.RemoveAllerrors with_ =. Since this helper is now shared across build/prebuild cleanup and workspace seeding cleanup-on-failure paths, silent failures (e.g., permission denied, busy file) could leave stale artifacts without any diagnostic trail.♻️ Suggested improvement to surface failures via logging
-func RemoveBuildArtifacts(contextPath string) { +func RemoveBuildArtifacts(contextPath string) { for _, name := range buildArtifactNames { - _ = os.RemoveAll(filepath.Join(contextPath, name)) + if err := os.RemoveAll(filepath.Join(contextPath, name)); err != nil { + log.Debugf("failed to remove build artifact %s: %v", name, err) + } } }🤖 Prompt for 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. In `@pkg/devcontainer/config/artifacts.go` around lines 19 - 23, RemoveBuildArtifacts is discarding cleanup failures from os.RemoveAll, so update this helper to surface errors instead of ignoring them. Use the existing RemoveBuildArtifacts symbol in artifacts.go to either return an error or log each failure with enough context (artifact name and path) so callers in build/prebuild cleanup and workspace seeding can detect and diagnose stale-artifact problems.
🤖 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 `@pkg/agent/delivery/workspace_seed.go`:
- Around line 56-64: The workspace seeding flow in
createVolume/copyDirIntoVolume marks the volume as seeded too early by setting
pkgconfig.DockerSeededLabel before the copy finishes. Move the label assignment
so it only happens after copyDirIntoVolume succeeds, and in the failure path for
removeVolume either propagate the cleanup error or otherwise prevent the partial
volume from being treated as complete on the next run.
- Around line 121-129: The volumeSeedState logic now depends only on Docker
labels, so legacy managed volumes that predate DockerSeededLabel are being
misclassified as unseeded and reseeded. Update volumeSeedState to handle
existing managed volumes conservatively by detecting the missing seeded label
and preserving seeded state unless an explicit reset path is taken, or add a
migration/fallback for legacy volumes. Use the Docker inspect parsing in
volumeSeedState and the managed/seeded label checks to keep backward
compatibility.
---
Nitpick comments:
In `@pkg/devcontainer/config/artifacts.go`:
- Around line 19-23: RemoveBuildArtifacts is discarding cleanup failures from
os.RemoveAll, so update this helper to surface errors instead of ignoring them.
Use the existing RemoveBuildArtifacts symbol in artifacts.go to either return an
error or log each failure with enough context (artifact name and path) so
callers in build/prebuild cleanup and workspace seeding can detect and diagnose
stale-artifact problems.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e43e988-8651-4315-a611-59d9f718b03c
📒 Files selected for processing (8)
pkg/agent/delivery/local_docker_test.gopkg/agent/delivery/workspace_seed.gopkg/agent/tunnelserver/tunnelserver.gopkg/devcontainer/build.gopkg/devcontainer/config/artifacts.gopkg/devcontainer/config/artifacts_test.gopkg/devcontainer/config/prebuild.gopkg/devcontainer/prebuild.go
Comment on lines
+121
to
+129
| "--format", "{{index .Labels \""+pkgconfig.DockerManagedLabel+"\"}},"+ | ||
| "{{index .Labels \""+pkgconfig.DockerSeededLabel+"\"}}", | ||
| name, | ||
| ).CombinedOutput() | ||
| if err != nil { | ||
| return false, false, fmt.Errorf("inspect volume %s: %s: %w", name, string(out), err) | ||
| } | ||
| managed = strings.TrimSpace(string(out)) == "true" | ||
|
|
||
| return managed, d.volumeSeeded(ctx, name), nil | ||
| } | ||
|
|
||
| // volumeSeeded reports whether the seeded sentinel file exists in the volume. | ||
| func (d *LocalDockerDelivery) volumeSeeded(ctx context.Context, name string) bool { | ||
| args := []string{ | ||
| cmdRun, flagRM, | ||
| "-v", name + ":/target:ro", | ||
| d.helperImageName(), | ||
| "sh", "-c", "[ -e /target/" + seededSentinel + " ]", | ||
| } | ||
| return d.cmd(ctx, args...).Run() == nil | ||
| managedStr, seededStr, _ := strings.Cut(strings.TrimSpace(string(out)), ",") | ||
| return managedStr == pkgconfig.LabelValueTrue, seededStr == pkgconfig.LabelValueTrue, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any legacy sentinel fallback remains around the new label-only path.
rg -n '\.devsy-seeded|seededSentinel|DockerSeededLabel|volumeSeedState|prepareSeedTarget' .Repository: devsy-org/devsy
Length of output: 1012
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' pkg/agent/delivery/workspace_seed.goRepository: devsy-org/devsy
Length of output: 5308
🏁 Script executed:
#!/bin/bash
rg -n "func .*createVolume|createVolume\\(|DockerManagedLabel|DockerSeededLabel|devsy-seeded|seeded" pkg/agent/delivery pkg/config -g '!**/*_test.go'Repository: devsy-org/devsy
Length of output: 2376
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' pkg/agent/delivery/local_docker.goRepository: devsy-org/devsy
Length of output: 5915
🏁 Script executed:
#!/bin/bash
rg -n "workspace volume|seed workspace|already seeded|--reset|sentinel|devsy-seeded|seeded volume" README.md docs pkg -g '!**/*_test.go'Repository: devsy-org/devsy
Length of output: 2478
🏁 Script executed:
#!/bin/bash
sed -n '450,540p' pkg/devcontainer/single.goRepository: devsy-org/devsy
Length of output: 3096
Preserve seeded-state compatibility for existing managed volumes. volumeSeedState now relies only on Docker labels, so a managed volume created before DockerSeededLabel existed is treated as unseeded and gets reseeded in place. Add a migration/fallback for legacy seeded volumes, or treat managed volumes missing the new label conservatively unless --reset is set.
🤖 Prompt for 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.
In `@pkg/agent/delivery/workspace_seed.go` around lines 121 - 129, The
volumeSeedState logic now depends only on Docker labels, so legacy managed
volumes that predate DockerSeededLabel are being misclassified as unseeded and
reseeded. Update volumeSeedState to handle existing managed volumes
conservatively by detecting the missing seeded label and preserving seeded state
unless an explicit reset path is taken, or add a migration/fallback for legacy
volumes. Use the Docker inspect parsing in volumeSeedState and the
managed/seeded label checks to keep backward compatibility.
…failures Volumes seeded before the seeded label existed are marked only by the legacy .devsy-seeded sentinel file; recognize it in volumeSeedState so GA-created (v1.5.0) workspace volumes are not re-seeded over user changes. When a seed copy fails and the follow-up volume removal also fails, join both errors instead of swallowing the cleanup failure, so a partial volume is never silently treated as complete. Log RemoveBuildArtifacts failures at debug for a diagnostic trail.
Backward compatibility with pre-label seeded volumes is not required; volumeSeedState relies solely on the seeded label.
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
Follow-up fix for the workspace-volume seeding added in #605. On a seeded workspace,
git statusshowed devsy artifacts as uncommitted/untracked content:The workspace volume is mounted at the workspace folder, so the volume root is the git working tree. Two seed artifacts leaked into it:
.devsy-seeded— the seed marker was written as a file at the volume root. It is now tracked as ansh.devsy.seededvolume label instead (set at volume creation). The volume is removed on copy failure so the label never persists for an empty volume, preserving idempotent re-seed behavior..devsy-internal/— devsy's transient Dockerfile-build folder was captured by thecp -aseed (the build runs before the seed and its cleanup is deferred). The copy now usestarwith--excludefor that folder, preserving attributes/permissions.Verification
Verified on an SSH provider with a Dockerfile-based devcontainer:
.devsy-seeded, no.devsy-internal..git, file contents, and executable bits preserved.Summary by CodeRabbit