From d9f767d3c44c70866f6c6c0147cabc729619894c Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 11:29:15 -0500 Subject: [PATCH 1/9] fix(delivery): stop seed from polluting the workspace tree 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. --- pkg/agent/delivery/local_docker_test.go | 30 +++++++++++ pkg/agent/delivery/workspace_seed.go | 68 +++++++++---------------- 2 files changed, 54 insertions(+), 44 deletions(-) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 2e72bd013..850a0aa8b 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -415,3 +415,33 @@ func TestLocalDockerDelivery_Cleanup_RemovesManagedVolumes(t *testing.T) { assert.Contains(t, removed, "devsy-agent-ws1") assert.Contains(t, removed, "ws1-workspace") } + +func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { + tmpDir := t.TempDir() + logPath := filepath.Join(tmpDir, "run.log") + + // Fake docker records the `run` invocation so the test can assert the copy + // command excludes devsy's build-internal folder. + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 1; fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"create\" ]; then exit 0; fi\n" + + "if [ \"$1\" = \"run\" ]; then echo \"$@\" >> \"" + logPath + "\"; exit 0; fi\n" + + "exit 0\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + d := &LocalDockerDelivery{DockerCommand: scriptPath} + err := d.SeedWorkspaceVolume(context.Background(), WorkspaceSeedOptions{ + WorkspaceID: "ws1", + VolumeName: "ws1-workspace", + SourceDir: "/local/src", + }) + require.NoError(t, err) + + logged, err := os.ReadFile(logPath) //nolint:gosec // test reads a temp file we control + require.NoError(t, err) + assert.Contains(t, string(logged), "--exclude") + assert.Contains(t, string(logged), ".devsy-internal") +} diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index 9d50fcf4c..9748ee41d 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -51,19 +51,24 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( return nil } + // The seeded state is recorded as a volume label rather than a file so + // nothing is written into the workspace tree (the volume is mounted at the + // workspace folder, so a file at its root would appear as untracked content). labels := pkgconfig.DockerVolumeLabels(opts.WorkspaceID, pkgconfig.VolumeRoleWorkspace) + labels[pkgconfig.DockerSeededLabel] = pkgconfig.LabelValueTrue if err := d.createVolume(ctx, opts.VolumeName, labels); err != nil { return fmt.Errorf("create workspace volume: %w", err) } if err := d.copyDirIntoVolume(ctx, opts.SourceDir, opts.VolumeName); err != nil { - // Leave the volume in place but unseeded so a retry re-copies. + // Remove the volume so its seeded label does not persist for an empty + // volume; the next up then re-creates and re-seeds cleanly. + if rmErr := d.removeVolume(ctx, opts.VolumeName); rmErr != nil { + log.Debugf("failed to remove volume after seed failure: %v", rmErr) + } return fmt.Errorf("seed workspace volume: %w", err) } - if err := d.markVolumeSeeded(ctx, opts.VolumeName); err != nil { - log.Debugf("failed to mark volume %s seeded: %v", opts.VolumeName, err) - } log.Infof("seeded workspace volume %s from %s", opts.VolumeName, opts.SourceDir) return nil } @@ -107,10 +112,8 @@ func (d *LocalDockerDelivery) prepareSeedTarget( return true, nil } -// volumeSeedState reports whether the volume is devsy-managed (from its label) -// and whether it has already been seeded (from a sentinel file written inside -// the volume after a successful copy). Docker cannot add labels to an existing -// volume, so seeded state is tracked with the sentinel rather than a label. +// volumeSeedState reports whether the volume is devsy-managed and whether it +// has already been seeded, both read from the volume's labels. func (d *LocalDockerDelivery) volumeSeedState( ctx context.Context, name string, @@ -121,26 +124,15 @@ func (d *LocalDockerDelivery) volumeSeedState( out, err := d.cmd(ctx, "volume", "inspect", - "--format", "{{index .Labels \""+pkgconfig.DockerManagedLabel+"\"}}", + "--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 } func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) bool { @@ -149,34 +141,24 @@ func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) boo } // copyDirIntoVolume copies the contents of sourceDir into the volume root using -// a throwaway helper container. The source is bind-mounted read-only. +// a throwaway helper container. The source is bind-mounted read-only. devsy's +// own transient build artifacts are excluded so they do not appear as untracked +// files in the seeded workspace tree. func (d *LocalDockerDelivery) copyDirIntoVolume( ctx context.Context, sourceDir, volumeName string, ) error { + // Preserve attributes (-p), recurse into everything, and drop devsy's + // build-internal folder wherever it appears in the tree. + script := "cd /source && tar -c" + + " --exclude='" + pkgconfig.ConfigDirName + "-internal'" + + " . | tar -x -C /target" args := []string{ cmdRun, flagRM, "-v", sourceDir + ":/source:ro", "-v", volumeName + ":/target", d.helperImageName(), - "sh", "-c", "cp -a /source/. /target/", - } - out, err := d.cmd(ctx, args...).CombinedOutput() - if err != nil { - return fmt.Errorf("%s: %w", string(out), err) - } - return nil -} - -// markVolumeSeeded records that the volume has been populated by writing a -// sentinel file inside it. Docker cannot add labels to an existing volume, so -// the sentinel is the source of truth for seeded state. -func (d *LocalDockerDelivery) markVolumeSeeded(ctx context.Context, volumeName string) error { - args := []string{ - cmdRun, flagRM, - "-v", volumeName + ":/target", - d.helperImageName(), - "sh", "-c", "touch /target/" + seededSentinel, + "sh", "-c", script, } out, err := d.cmd(ctx, args...).CombinedOutput() if err != nil { @@ -184,5 +166,3 @@ func (d *LocalDockerDelivery) markVolumeSeeded(ctx context.Context, volumeName s } return nil } - -const seededSentinel = ".devsy-seeded" From 0233e84d4e1db19364fb2c095f47ebe900687227 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 12:05:34 -0500 Subject: [PATCH 2/9] refactor(devcontainer): centralize build-artifact handling and clean 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. --- pkg/agent/delivery/workspace_seed.go | 17 ++++--- pkg/devcontainer/build.go | 11 ++++- pkg/devcontainer/config/artifacts.go | 34 ++++++++++++++ pkg/devcontainer/config/artifacts_test.go | 56 +++++++++++++++++++++++ pkg/devcontainer/config/prebuild.go | 6 ++- pkg/devcontainer/prebuild.go | 9 +--- 6 files changed, 117 insertions(+), 16 deletions(-) create mode 100644 pkg/devcontainer/config/artifacts.go create mode 100644 pkg/devcontainer/config/artifacts_test.go diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index 9748ee41d..6ed9becd6 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -6,6 +6,7 @@ import ( "strings" pkgconfig "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" ) @@ -148,17 +149,21 @@ func (d *LocalDockerDelivery) copyDirIntoVolume( ctx context.Context, sourceDir, volumeName string, ) error { - // Preserve attributes (-p), recurse into everything, and drop devsy's - // build-internal folder wherever it appears in the tree. - script := "cd /source && tar -c" + - " --exclude='" + pkgconfig.ConfigDirName + "-internal'" + - " . | tar -x -C /target" + // tar preserves attributes and recurses; excluding devsy's build artifacts + // keeps its scaffolding out of the seeded workspace tree. As a safety net — + // build cleanup should already have removed them before seeding runs. + var script strings.Builder + script.WriteString("cd /source && tar -c") + for _, artifact := range config.BuildArtifactExcludes() { + script.WriteString(" --exclude='" + artifact + "'") + } + script.WriteString(" . | tar -x -C /target") args := []string{ cmdRun, flagRM, "-v", sourceDir + ":/source:ro", "-v", volumeName + ":/target", d.helperImageName(), - "sh", "-c", script, + "sh", "-c", script.String(), } out, err := d.cmd(ctx, args...).CombinedOutput() if err != nil { diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index cb3b03e50..7776a8b65 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -43,6 +43,14 @@ func (r *runner) build( return nil, err } + // The build has consumed the on-disk artifacts (feature scaffolding, + // generated Dockerfiles), so remove them eagerly here rather than relying + // solely on the deferred cleanup in Up. This ensures they are gone before + // any later step that copies the workspace tree (e.g. volume seeding), so + // devsy's scaffolding never leaks into the workspace. The deferred cleanup + // remains as a safety net for the error paths above. + config.RemoveBuildArtifacts(config.GetContextPath(parsedConfig.Config)) + // Add extra devcontainer config if provided if options.ExtraDevContainerPath != "" { if buildInfo.ImageMetadata == nil { @@ -672,8 +680,7 @@ func getContainerContextAndDockerfile( } func cleanupBuildInformation(c *config.DevContainerConfig) { - contextPath := config.GetContextPath(c) - _ = os.RemoveAll(filepath.Join(contextPath, config.DevsyContextFeatureFolder)) + config.RemoveBuildArtifacts(config.GetContextPath(c)) } func featureSecretOpts(options provider.BuildOptions) *feature.SecretOptions { diff --git a/pkg/devcontainer/config/artifacts.go b/pkg/devcontainer/config/artifacts.go new file mode 100644 index 000000000..8f010c368 --- /dev/null +++ b/pkg/devcontainer/config/artifacts.go @@ -0,0 +1,34 @@ +package config + +import ( + "os" + "path/filepath" +) + +// buildArtifactNames are the workspace-relative names devsy writes into the +// build context for its own bookkeeping (feature scaffolding, generated +// Dockerfiles, etc.). They are transient build-time artifacts, never workspace +// content, and never needed at runtime. +// +// This is the single source of truth: anything that copies, hashes, or cleans +// the workspace/build-context tree must consult these helpers rather than +// hard-coding names, so a new artifact only needs to be added here. +var buildArtifactNames = []string{ + DevsyContextFeatureFolder, +} + +// BuildArtifactExcludes returns the artifact names to exclude when copying or +// hashing a tree (e.g. workspace-volume seeding, prebuild-hash computation). +// Names are relative to the tree root; callers apply them per their own +// matching syntax (tar --exclude, dockerignore, etc.). +func BuildArtifactExcludes() []string { + return append([]string(nil), buildArtifactNames...) +} + +// RemoveBuildArtifacts deletes devsy's build artifacts from contextPath. It is +// best-effort and safe to call when they are absent. +func RemoveBuildArtifacts(contextPath string) { + for _, name := range buildArtifactNames { + _ = os.RemoveAll(filepath.Join(contextPath, name)) + } +} diff --git a/pkg/devcontainer/config/artifacts_test.go b/pkg/devcontainer/config/artifacts_test.go new file mode 100644 index 000000000..83dd5ec2b --- /dev/null +++ b/pkg/devcontainer/config/artifacts_test.go @@ -0,0 +1,56 @@ +package config + +import ( + "os" + "path/filepath" + "slices" + "testing" +) + +func TestBuildArtifactExcludesIncludesFeatureFolder(t *testing.T) { + got := BuildArtifactExcludes() + if !slices.Contains(got, DevsyContextFeatureFolder) { + t.Errorf("expected %q in excludes, got %v", DevsyContextFeatureFolder, got) + } +} + +func TestBuildArtifactExcludesReturnsCopy(t *testing.T) { + // Mutating the returned slice must not affect the source of truth. + got := BuildArtifactExcludes() + if len(got) == 0 { + t.Fatal("expected at least one artifact") + } + got[0] = "mutated" + if BuildArtifactExcludes()[0] == "mutated" { + t.Error("BuildArtifactExcludes must return a defensive copy") + } +} + +func TestRemoveBuildArtifacts(t *testing.T) { + contextPath := t.TempDir() + artifactDir := filepath.Join(contextPath, DevsyContextFeatureFolder) + // #nosec G301 -- test fixture + if err := os.MkdirAll(filepath.Join(artifactDir, "0"), 0o755); err != nil { + t.Fatal(err) + } + // A real workspace file that must survive. + keep := filepath.Join(contextPath, "keep.txt") + // #nosec G306 -- test fixture + if err := os.WriteFile(keep, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + + RemoveBuildArtifacts(contextPath) + + if _, err := os.Stat(artifactDir); !os.IsNotExist(err) { + t.Errorf("expected artifact dir removed, stat err = %v", err) + } + if _, err := os.Stat(keep); err != nil { + t.Errorf("workspace file should survive, got %v", err) + } +} + +func TestRemoveBuildArtifactsAbsentIsNoError(t *testing.T) { + // Safe to call when nothing exists. + RemoveBuildArtifacts(t.TempDir()) +} diff --git a/pkg/devcontainer/config/prebuild.go b/pkg/devcontainer/config/prebuild.go index 479d6b72e..964feeb00 100644 --- a/pkg/devcontainer/config/prebuild.go +++ b/pkg/devcontainer/config/prebuild.go @@ -42,7 +42,11 @@ func CalculatePrebuildHash(params PrebuildHashParams) (string, error) { log.Debugf("failed to read .dockerignore: %v", err) return "", fmt.Errorf("failed to read dockerignore: %w", err) } - excludes = append(excludes, DevsyContextFeatureFolder+"/") + // devsy's own build artifacts are recreated per build, so exclude them from + // the prebuild hash to keep the cache key stable. + for _, artifact := range BuildArtifactExcludes() { + excludes = append(excludes, artifact+"/") + } var includes []string if params.BuildInfo != nil && params.BuildInfo.Dockerfile != nil { diff --git a/pkg/devcontainer/prebuild.go b/pkg/devcontainer/prebuild.go index ebfc876b4..e36b8c43f 100644 --- a/pkg/devcontainer/prebuild.go +++ b/pkg/devcontainer/prebuild.go @@ -3,8 +3,6 @@ package devcontainer import ( "context" "fmt" - "os" - "path/filepath" "strings" "github.com/devsy-org/devsy/pkg/devcontainer/build" @@ -27,11 +25,8 @@ func (r *runner) Build(ctx context.Context, options provider.BuildOptions) (stri prebuildRepo := getPrebuildRepository(substitutedConfig) - // remove build information - defer func() { - contextPath := config.GetContextPath(substitutedConfig.Config) - _ = os.RemoveAll(filepath.Join(contextPath, config.DevsyContextFeatureFolder)) - }() + // remove build information (safety net; build() also cleans up eagerly) + defer config.RemoveBuildArtifacts(config.GetContextPath(substitutedConfig.Config)) // check if we need to build container buildInfo, err := r.build(ctx, substitutedConfig, substitutionContext, options) From 948f8a8e48fa0f32a0623123cb8aa5cd6a7be06d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 12:13:40 -0500 Subject: [PATCH 3/9] docs: trim build-artifact handling comments to concise intent 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. --- pkg/agent/delivery/workspace_seed.go | 17 ++++++----------- pkg/devcontainer/build.go | 9 +++------ pkg/devcontainer/config/artifacts.go | 21 +++++++-------------- pkg/devcontainer/config/prebuild.go | 3 +-- 4 files changed, 17 insertions(+), 33 deletions(-) diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index 6ed9becd6..6633990e9 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -52,9 +52,8 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( return nil } - // The seeded state is recorded as a volume label rather than a file so - // nothing is written into the workspace tree (the volume is mounted at the - // workspace folder, so a file at its root would appear as untracked content). + // Seeded state is a label, not a file: the volume mounts at the workspace + // folder, so a marker file at its root would show up as untracked content. labels := pkgconfig.DockerVolumeLabels(opts.WorkspaceID, pkgconfig.VolumeRoleWorkspace) labels[pkgconfig.DockerSeededLabel] = pkgconfig.LabelValueTrue if err := d.createVolume(ctx, opts.VolumeName, labels); err != nil { @@ -63,7 +62,7 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( if err := d.copyDirIntoVolume(ctx, opts.SourceDir, opts.VolumeName); err != nil { // Remove the volume so its seeded label does not persist for an empty - // volume; the next up then re-creates and re-seeds cleanly. + // volume; the next up re-creates and re-seeds. if rmErr := d.removeVolume(ctx, opts.VolumeName); rmErr != nil { log.Debugf("failed to remove volume after seed failure: %v", rmErr) } @@ -141,17 +140,13 @@ func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) boo return err == nil } -// copyDirIntoVolume copies the contents of sourceDir into the volume root using -// a throwaway helper container. The source is bind-mounted read-only. devsy's -// own transient build artifacts are excluded so they do not appear as untracked -// files in the seeded workspace tree. +// copyDirIntoVolume copies sourceDir into the volume root via a throwaway +// helper container, excluding devsy's build artifacts so they never surface as +// untracked files in the seeded workspace tree. func (d *LocalDockerDelivery) copyDirIntoVolume( ctx context.Context, sourceDir, volumeName string, ) error { - // tar preserves attributes and recurses; excluding devsy's build artifacts - // keeps its scaffolding out of the seeded workspace tree. As a safety net — - // build cleanup should already have removed them before seeding runs. var script strings.Builder script.WriteString("cd /source && tar -c") for _, artifact := range config.BuildArtifactExcludes() { diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 7776a8b65..1d08be954 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -43,12 +43,9 @@ func (r *runner) build( return nil, err } - // The build has consumed the on-disk artifacts (feature scaffolding, - // generated Dockerfiles), so remove them eagerly here rather than relying - // solely on the deferred cleanup in Up. This ensures they are gone before - // any later step that copies the workspace tree (e.g. volume seeding), so - // devsy's scaffolding never leaks into the workspace. The deferred cleanup - // remains as a safety net for the error paths above. + // Remove artifacts eagerly so they are gone before any later step copies + // the workspace tree (e.g. volume seeding); the deferred cleanup in Up + // covers the error paths above. config.RemoveBuildArtifacts(config.GetContextPath(parsedConfig.Config)) // Add extra devcontainer config if provided diff --git a/pkg/devcontainer/config/artifacts.go b/pkg/devcontainer/config/artifacts.go index 8f010c368..01b526614 100644 --- a/pkg/devcontainer/config/artifacts.go +++ b/pkg/devcontainer/config/artifacts.go @@ -5,28 +5,21 @@ import ( "path/filepath" ) -// buildArtifactNames are the workspace-relative names devsy writes into the -// build context for its own bookkeeping (feature scaffolding, generated -// Dockerfiles, etc.). They are transient build-time artifacts, never workspace -// content, and never needed at runtime. -// -// This is the single source of truth: anything that copies, hashes, or cleans -// the workspace/build-context tree must consult these helpers rather than -// hard-coding names, so a new artifact only needs to be added here. +// buildArtifactNames is the single source of truth for the transient files +// devsy writes into the build context. Consumers that copy, hash, or clean the +// tree consult the helpers below rather than hard-coding names. var buildArtifactNames = []string{ DevsyContextFeatureFolder, } -// BuildArtifactExcludes returns the artifact names to exclude when copying or -// hashing a tree (e.g. workspace-volume seeding, prebuild-hash computation). -// Names are relative to the tree root; callers apply them per their own -// matching syntax (tar --exclude, dockerignore, etc.). +// BuildArtifactExcludes returns the artifact names, relative to the tree root, +// for callers to exclude when copying or hashing (tar, dockerignore, etc.). func BuildArtifactExcludes() []string { return append([]string(nil), buildArtifactNames...) } -// RemoveBuildArtifacts deletes devsy's build artifacts from contextPath. It is -// best-effort and safe to call when they are absent. +// RemoveBuildArtifacts deletes devsy's build artifacts from contextPath. +// Best-effort and safe to call when they are absent. func RemoveBuildArtifacts(contextPath string) { for _, name := range buildArtifactNames { _ = os.RemoveAll(filepath.Join(contextPath, name)) diff --git a/pkg/devcontainer/config/prebuild.go b/pkg/devcontainer/config/prebuild.go index 964feeb00..6d0db1bfe 100644 --- a/pkg/devcontainer/config/prebuild.go +++ b/pkg/devcontainer/config/prebuild.go @@ -42,8 +42,7 @@ func CalculatePrebuildHash(params PrebuildHashParams) (string, error) { log.Debugf("failed to read .dockerignore: %v", err) return "", fmt.Errorf("failed to read dockerignore: %w", err) } - // devsy's own build artifacts are recreated per build, so exclude them from - // the prebuild hash to keep the cache key stable. + // Artifacts are recreated per build; exclude them to keep the hash stable. for _, artifact := range BuildArtifactExcludes() { excludes = append(excludes, artifact+"/") } From ac30020379d411f12884f18f37a78a37cb08191d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 12:50:55 -0500 Subject: [PATCH 4/9] fix(build): skip eager artifact cleanup for dockerless builds 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. --- pkg/devcontainer/build.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 1d08be954..09e2b2871 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -45,8 +45,12 @@ func (r *runner) build( // Remove artifacts eagerly so they are gone before any later step copies // the workspace tree (e.g. volume seeding); the deferred cleanup in Up - // covers the error paths above. - config.RemoveBuildArtifacts(config.GetContextPath(parsedConfig.Config)) + // covers the error paths above. Dockerless builds are the exception: the + // image is built later inside the container, so their artifacts must + // survive and are cleaned up by the dockerless flow itself. + if buildInfo.Dockerless == nil { + config.RemoveBuildArtifacts(config.GetContextPath(parsedConfig.Config)) + } // Add extra devcontainer config if provided if options.ExtraDevContainerPath != "" { From a7b113433d3e340e3aca9b1c7938afa0e8884d35 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 13:21:57 -0500 Subject: [PATCH 5/9] refactor(build): make artifact exclusion idempotent, drop eager cleanup 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. --- pkg/agent/tunnelserver/tunnelserver.go | 6 ++++++ pkg/devcontainer/build.go | 9 --------- pkg/devcontainer/config/artifacts.go | 12 ++++++++---- pkg/devcontainer/prebuild.go | 3 ++- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/pkg/agent/tunnelserver/tunnelserver.go b/pkg/agent/tunnelserver/tunnelserver.go index 637fcf4b3..1ef1eb209 100644 --- a/pkg/agent/tunnelserver/tunnelserver.go +++ b/pkg/agent/tunnelserver/tunnelserver.go @@ -453,6 +453,9 @@ func (t *tunnelServer) StreamWorkspace( } } + // Keep transient build artifacts out of the workspace tree we ship remote. + excludes = append(excludes, config.BuildArtifactExcludes()...) + buf := bufio.NewWriterSize(NewStreamWriter(stream), 10*1024) err = extract.WriteTarExclude(buf, t.workspace.Source.LocalFolder, false, excludes) if err != nil { @@ -497,6 +500,9 @@ func (t *tunnelServer) StreamMount( } } + // Keep transient build artifacts out of the mount tree we ship remote. + excludes = append(excludes, config.BuildArtifactExcludes()...) + buf := bufio.NewWriterSize(NewStreamWriter(stream), 10*1024) err := extract.WriteTarExclude(buf, mount.Source, false, excludes) if err != nil { diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 09e2b2871..2c137226a 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -43,15 +43,6 @@ func (r *runner) build( return nil, err } - // Remove artifacts eagerly so they are gone before any later step copies - // the workspace tree (e.g. volume seeding); the deferred cleanup in Up - // covers the error paths above. Dockerless builds are the exception: the - // image is built later inside the container, so their artifacts must - // survive and are cleaned up by the dockerless flow itself. - if buildInfo.Dockerless == nil { - config.RemoveBuildArtifacts(config.GetContextPath(parsedConfig.Config)) - } - // Add extra devcontainer config if provided if options.ExtraDevContainerPath != "" { if buildInfo.ImageMetadata == nil { diff --git a/pkg/devcontainer/config/artifacts.go b/pkg/devcontainer/config/artifacts.go index 01b526614..4f902709d 100644 --- a/pkg/devcontainer/config/artifacts.go +++ b/pkg/devcontainer/config/artifacts.go @@ -6,20 +6,24 @@ import ( ) // buildArtifactNames is the single source of truth for the transient files -// devsy writes into the build context. Consumers that copy, hash, or clean the -// tree consult the helpers below rather than hard-coding names. +// devsy writes into the build context. +// +// Invariant: any operation that copies, streams, or hashes a workspace tree +// must exclude these via BuildArtifactExcludes. That exclusion — not the +// best-effort RemoveBuildArtifacts cleanup — is what keeps artifacts out of a +// user's workspace, so correctness never depends on cleanup timing. var buildArtifactNames = []string{ DevsyContextFeatureFolder, } // BuildArtifactExcludes returns the artifact names, relative to the tree root, -// for callers to exclude when copying or hashing (tar, dockerignore, etc.). +// for callers to exclude when copying, streaming, or hashing the tree. func BuildArtifactExcludes() []string { return append([]string(nil), buildArtifactNames...) } // RemoveBuildArtifacts deletes devsy's build artifacts from contextPath. -// Best-effort and safe to call when they are absent. +// Best-effort tidiness only; see the invariant on buildArtifactNames. func RemoveBuildArtifacts(contextPath string) { for _, name := range buildArtifactNames { _ = os.RemoveAll(filepath.Join(contextPath, name)) diff --git a/pkg/devcontainer/prebuild.go b/pkg/devcontainer/prebuild.go index e36b8c43f..272b3d883 100644 --- a/pkg/devcontainer/prebuild.go +++ b/pkg/devcontainer/prebuild.go @@ -25,7 +25,8 @@ func (r *runner) Build(ctx context.Context, options provider.BuildOptions) (stri prebuildRepo := getPrebuildRepository(substitutedConfig) - // remove build information (safety net; build() also cleans up eagerly) + // Tidy up transient build artifacts once the build completes; correctness + // relies on the exclude at copy time, not on this cleanup. defer config.RemoveBuildArtifacts(config.GetContextPath(substitutedConfig.Config)) // check if we need to build container From f709c8987d2b7627662fd100092c90d2270830e0 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 14:06:35 -0500 Subject: [PATCH 6/9] chore: trim verbose comments added on this branch --- pkg/agent/delivery/local_docker_test.go | 2 -- pkg/agent/delivery/workspace_seed.go | 9 +-------- pkg/agent/tunnelserver/tunnelserver.go | 2 -- pkg/devcontainer/config/artifacts.go | 12 ++---------- pkg/devcontainer/config/artifacts_test.go | 3 --- pkg/devcontainer/config/prebuild.go | 1 - pkg/devcontainer/prebuild.go | 2 -- 7 files changed, 3 insertions(+), 28 deletions(-) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 850a0aa8b..117f43695 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -420,8 +420,6 @@ func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { tmpDir := t.TempDir() logPath := filepath.Join(tmpDir, "run.log") - // Fake docker records the `run` invocation so the test can assert the copy - // command excludes devsy's build-internal folder. scriptPath := filepath.Join(tmpDir, "fake-docker.sh") script := "#!/bin/sh\n" + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 1; fi\n" + diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index 6633990e9..59599e895 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -52,8 +52,6 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( return nil } - // Seeded state is a label, not a file: the volume mounts at the workspace - // folder, so a marker file at its root would show up as untracked content. labels := pkgconfig.DockerVolumeLabels(opts.WorkspaceID, pkgconfig.VolumeRoleWorkspace) labels[pkgconfig.DockerSeededLabel] = pkgconfig.LabelValueTrue if err := d.createVolume(ctx, opts.VolumeName, labels); err != nil { @@ -61,8 +59,6 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( } if err := d.copyDirIntoVolume(ctx, opts.SourceDir, opts.VolumeName); err != nil { - // Remove the volume so its seeded label does not persist for an empty - // volume; the next up re-creates and re-seeds. if rmErr := d.removeVolume(ctx, opts.VolumeName); rmErr != nil { log.Debugf("failed to remove volume after seed failure: %v", rmErr) } @@ -112,8 +108,6 @@ func (d *LocalDockerDelivery) prepareSeedTarget( return true, nil } -// volumeSeedState reports whether the volume is devsy-managed and whether it -// has already been seeded, both read from the volume's labels. func (d *LocalDockerDelivery) volumeSeedState( ctx context.Context, name string, @@ -141,8 +135,7 @@ func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) boo } // copyDirIntoVolume copies sourceDir into the volume root via a throwaway -// helper container, excluding devsy's build artifacts so they never surface as -// untracked files in the seeded workspace tree. +// helper container, excluding devsy's build artifacts. func (d *LocalDockerDelivery) copyDirIntoVolume( ctx context.Context, sourceDir, volumeName string, diff --git a/pkg/agent/tunnelserver/tunnelserver.go b/pkg/agent/tunnelserver/tunnelserver.go index 1ef1eb209..cddac4942 100644 --- a/pkg/agent/tunnelserver/tunnelserver.go +++ b/pkg/agent/tunnelserver/tunnelserver.go @@ -453,7 +453,6 @@ func (t *tunnelServer) StreamWorkspace( } } - // Keep transient build artifacts out of the workspace tree we ship remote. excludes = append(excludes, config.BuildArtifactExcludes()...) buf := bufio.NewWriterSize(NewStreamWriter(stream), 10*1024) @@ -500,7 +499,6 @@ func (t *tunnelServer) StreamMount( } } - // Keep transient build artifacts out of the mount tree we ship remote. excludes = append(excludes, config.BuildArtifactExcludes()...) buf := bufio.NewWriterSize(NewStreamWriter(stream), 10*1024) diff --git a/pkg/devcontainer/config/artifacts.go b/pkg/devcontainer/config/artifacts.go index 4f902709d..7717f9a1a 100644 --- a/pkg/devcontainer/config/artifacts.go +++ b/pkg/devcontainer/config/artifacts.go @@ -5,25 +5,17 @@ import ( "path/filepath" ) -// buildArtifactNames is the single source of truth for the transient files -// devsy writes into the build context. -// -// Invariant: any operation that copies, streams, or hashes a workspace tree -// must exclude these via BuildArtifactExcludes. That exclusion — not the -// best-effort RemoveBuildArtifacts cleanup — is what keeps artifacts out of a -// user's workspace, so correctness never depends on cleanup timing. var buildArtifactNames = []string{ DevsyContextFeatureFolder, } -// BuildArtifactExcludes returns the artifact names, relative to the tree root, -// for callers to exclude when copying, streaming, or hashing the tree. +// BuildArtifactExcludes returns the build artifact names to exclude when +// copying, streaming, or hashing a workspace tree. func BuildArtifactExcludes() []string { return append([]string(nil), buildArtifactNames...) } // RemoveBuildArtifacts deletes devsy's build artifacts from contextPath. -// Best-effort tidiness only; see the invariant on buildArtifactNames. func RemoveBuildArtifacts(contextPath string) { for _, name := range buildArtifactNames { _ = os.RemoveAll(filepath.Join(contextPath, name)) diff --git a/pkg/devcontainer/config/artifacts_test.go b/pkg/devcontainer/config/artifacts_test.go index 83dd5ec2b..a9573f012 100644 --- a/pkg/devcontainer/config/artifacts_test.go +++ b/pkg/devcontainer/config/artifacts_test.go @@ -15,7 +15,6 @@ func TestBuildArtifactExcludesIncludesFeatureFolder(t *testing.T) { } func TestBuildArtifactExcludesReturnsCopy(t *testing.T) { - // Mutating the returned slice must not affect the source of truth. got := BuildArtifactExcludes() if len(got) == 0 { t.Fatal("expected at least one artifact") @@ -33,7 +32,6 @@ func TestRemoveBuildArtifacts(t *testing.T) { if err := os.MkdirAll(filepath.Join(artifactDir, "0"), 0o755); err != nil { t.Fatal(err) } - // A real workspace file that must survive. keep := filepath.Join(contextPath, "keep.txt") // #nosec G306 -- test fixture if err := os.WriteFile(keep, []byte("x"), 0o644); err != nil { @@ -51,6 +49,5 @@ func TestRemoveBuildArtifacts(t *testing.T) { } func TestRemoveBuildArtifactsAbsentIsNoError(t *testing.T) { - // Safe to call when nothing exists. RemoveBuildArtifacts(t.TempDir()) } diff --git a/pkg/devcontainer/config/prebuild.go b/pkg/devcontainer/config/prebuild.go index 6d0db1bfe..0b787e24a 100644 --- a/pkg/devcontainer/config/prebuild.go +++ b/pkg/devcontainer/config/prebuild.go @@ -42,7 +42,6 @@ func CalculatePrebuildHash(params PrebuildHashParams) (string, error) { log.Debugf("failed to read .dockerignore: %v", err) return "", fmt.Errorf("failed to read dockerignore: %w", err) } - // Artifacts are recreated per build; exclude them to keep the hash stable. for _, artifact := range BuildArtifactExcludes() { excludes = append(excludes, artifact+"/") } diff --git a/pkg/devcontainer/prebuild.go b/pkg/devcontainer/prebuild.go index 272b3d883..410263d54 100644 --- a/pkg/devcontainer/prebuild.go +++ b/pkg/devcontainer/prebuild.go @@ -25,8 +25,6 @@ func (r *runner) Build(ctx context.Context, options provider.BuildOptions) (stri prebuildRepo := getPrebuildRepository(substitutedConfig) - // Tidy up transient build artifacts once the build completes; correctness - // relies on the exclude at copy time, not on this cleanup. defer config.RemoveBuildArtifacts(config.GetContextPath(substitutedConfig.Config)) // check if we need to build container From 3c048336f42aab727887ad264bac6ebc40ac9bde Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 14:51:42 -0500 Subject: [PATCH 7/9] fix(delivery): handle legacy seeded volumes and surface seed cleanup 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. --- pkg/agent/delivery/local_docker_test.go | 70 +++++++++++++++++++++++-- pkg/agent/delivery/workspace_seed.go | 37 ++++++++++++- pkg/devcontainer/config/artifacts.go | 7 ++- 3 files changed, 108 insertions(+), 6 deletions(-) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 117f43695..217c90521 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -16,6 +16,12 @@ import ( const testArch = "amd64" +const ( + testSeedWorkspaceID = "ws1" + testSeedVolumeName = "ws1-workspace" + testSeedSourceDir = "/local/src" +) + func TestLocalDockerDelivery_Phase(t *testing.T) { d := &LocalDockerDelivery{} assert.Equal(t, PhasePreStart, d.Phase()) @@ -432,9 +438,9 @@ func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { d := &LocalDockerDelivery{DockerCommand: scriptPath} err := d.SeedWorkspaceVolume(context.Background(), WorkspaceSeedOptions{ - WorkspaceID: "ws1", - VolumeName: "ws1-workspace", - SourceDir: "/local/src", + WorkspaceID: testSeedWorkspaceID, + VolumeName: testSeedVolumeName, + SourceDir: testSeedSourceDir, }) require.NoError(t, err) @@ -443,3 +449,61 @@ func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { assert.Contains(t, string(logged), "--exclude") assert.Contains(t, string(logged), ".devsy-internal") } + +func TestLocalDockerDelivery_Seed_LegacySentinelNotReseeded(t *testing.T) { + tmpDir := t.TempDir() + logPath := filepath.Join(tmpDir, "run.log") + + // Managed volume with no seeded label but the legacy sentinel present: + // inspect reports managed=true, seeded empty; the sentinel probe (run sh -c + // "[ -e ... ]") succeeds. Seeding must be skipped, so no copy runs. + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ] && [ \"$3\" = \"--format\" ]; then\n" + + " echo 'true,'; exit 0\n" + + "fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 0; fi\n" + + "if [ \"$1\" = \"run\" ]; then echo \"$@\" >> \"" + logPath + "\"; exit 0; fi\n" + + "exit 0\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + d := &LocalDockerDelivery{DockerCommand: scriptPath} + err := d.SeedWorkspaceVolume(context.Background(), WorkspaceSeedOptions{ + WorkspaceID: testSeedWorkspaceID, + VolumeName: testSeedVolumeName, + SourceDir: testSeedSourceDir, + }) + require.NoError(t, err) + + logged, _ := os.ReadFile(logPath) //nolint:gosec // test reads a temp file we control + assert.NotContains(t, string(logged), "/source", "copy must not run for a legacy-seeded volume") +} + +func TestLocalDockerDelivery_Seed_CopyAndCleanupFailureJoined(t *testing.T) { + tmpDir := t.TempDir() + + // Fresh volume (inspect fails), copy fails, and cleanup (volume rm) also + // fails: both errors must surface rather than be swallowed. + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 1; fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"create\" ]; then exit 0; fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"rm\" ]; then echo 'rm boom' 1>&2; exit 1; fi\n" + + "if [ \"$1\" = \"run\" ]; then echo 'copy boom' 1>&2; exit 1; fi\n" + + "exit 0\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + d := &LocalDockerDelivery{DockerCommand: scriptPath} + err := d.SeedWorkspaceVolume(context.Background(), WorkspaceSeedOptions{ + WorkspaceID: testSeedWorkspaceID, + VolumeName: testSeedVolumeName, + SourceDir: testSeedSourceDir, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "seed workspace volume") + assert.Contains(t, err.Error(), "remove partial volume") +} diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index 59599e895..29eadb4ab 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -2,6 +2,7 @@ package delivery import ( "context" + "errors" "fmt" "strings" @@ -10,6 +11,11 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) +// legacySeededSentinel is the pre-label marker file older devsy versions wrote +// into a seeded volume. Retained only to recognize volumes seeded before the +// seeded label existed. +const legacySeededSentinel = ".devsy-seeded" + // WorkspaceSeedOptions describes a request to seed a named workspace volume // from a local source directory. type WorkspaceSeedOptions struct { @@ -59,8 +65,14 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( } if err := d.copyDirIntoVolume(ctx, opts.SourceDir, opts.VolumeName); err != nil { + // The volume is now labeled seeded but holds partial contents. Removing + // it lets the next up re-seed; if removal also fails, surface both so a + // partial volume is never silently treated as complete. if rmErr := d.removeVolume(ctx, opts.VolumeName); rmErr != nil { - log.Debugf("failed to remove volume after seed failure: %v", rmErr) + return errors.Join( + fmt.Errorf("seed workspace volume: %w", err), + fmt.Errorf("remove partial volume %s: %w", opts.VolumeName, rmErr), + ) } return fmt.Errorf("seed workspace volume: %w", err) } @@ -126,7 +138,28 @@ func (d *LocalDockerDelivery) volumeSeedState( return false, false, fmt.Errorf("inspect volume %s: %s: %w", name, string(out), err) } managedStr, seededStr, _ := strings.Cut(strings.TrimSpace(string(out)), ",") - return managedStr == pkgconfig.LabelValueTrue, seededStr == pkgconfig.LabelValueTrue, nil + managed = managedStr == pkgconfig.LabelValueTrue + seeded = seededStr == pkgconfig.LabelValueTrue + + // Volumes seeded by older versions predate the seeded label and are only + // marked by a sentinel file; treat that as seeded so they are not + // re-seeded over user changes. + if managed && !seeded && d.volumeHasLegacySentinel(ctx, name) { + seeded = true + } + return managed, seeded, nil +} + +// volumeHasLegacySentinel reports whether the pre-label seeded marker file is +// present in the volume. +func (d *LocalDockerDelivery) volumeHasLegacySentinel(ctx context.Context, name string) bool { + args := []string{ + cmdRun, flagRM, + "-v", name + ":/target:ro", + d.helperImageName(), + "sh", "-c", "[ -e /target/" + legacySeededSentinel + " ]", + } + return d.cmd(ctx, args...).Run() == nil } func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) bool { diff --git a/pkg/devcontainer/config/artifacts.go b/pkg/devcontainer/config/artifacts.go index 7717f9a1a..244e2ec1c 100644 --- a/pkg/devcontainer/config/artifacts.go +++ b/pkg/devcontainer/config/artifacts.go @@ -3,6 +3,8 @@ package config import ( "os" "path/filepath" + + "github.com/devsy-org/devsy/pkg/log" ) var buildArtifactNames = []string{ @@ -18,6 +20,9 @@ func BuildArtifactExcludes() []string { // RemoveBuildArtifacts deletes devsy's build artifacts from contextPath. func RemoveBuildArtifacts(contextPath string) { for _, name := range buildArtifactNames { - _ = os.RemoveAll(filepath.Join(contextPath, name)) + path := filepath.Join(contextPath, name) + if err := os.RemoveAll(path); err != nil { + log.Debugf("failed to remove build artifact %s: %v", path, err) + } } } From 0cc9c6433387aea67e3f87dd20f0ed3aaa87b3ea Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 14:57:29 -0500 Subject: [PATCH 8/9] revert(delivery): drop legacy seeded-volume sentinel fallback Backward compatibility with pre-label seeded volumes is not required; volumeSeedState relies solely on the seeded label. --- pkg/agent/delivery/local_docker_test.go | 31 ------------------------- pkg/agent/delivery/workspace_seed.go | 28 +--------------------- 2 files changed, 1 insertion(+), 58 deletions(-) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 217c90521..c7972840a 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -450,37 +450,6 @@ func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { assert.Contains(t, string(logged), ".devsy-internal") } -func TestLocalDockerDelivery_Seed_LegacySentinelNotReseeded(t *testing.T) { - tmpDir := t.TempDir() - logPath := filepath.Join(tmpDir, "run.log") - - // Managed volume with no seeded label but the legacy sentinel present: - // inspect reports managed=true, seeded empty; the sentinel probe (run sh -c - // "[ -e ... ]") succeeds. Seeding must be skipped, so no copy runs. - scriptPath := filepath.Join(tmpDir, "fake-docker.sh") - script := "#!/bin/sh\n" + - "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ] && [ \"$3\" = \"--format\" ]; then\n" + - " echo 'true,'; exit 0\n" + - "fi\n" + - "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 0; fi\n" + - "if [ \"$1\" = \"run\" ]; then echo \"$@\" >> \"" + logPath + "\"; exit 0; fi\n" + - "exit 0\n" - require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) - // #nosec G302 -- test script must be executable - require.NoError(t, os.Chmod(scriptPath, 0o755)) - - d := &LocalDockerDelivery{DockerCommand: scriptPath} - err := d.SeedWorkspaceVolume(context.Background(), WorkspaceSeedOptions{ - WorkspaceID: testSeedWorkspaceID, - VolumeName: testSeedVolumeName, - SourceDir: testSeedSourceDir, - }) - require.NoError(t, err) - - logged, _ := os.ReadFile(logPath) //nolint:gosec // test reads a temp file we control - assert.NotContains(t, string(logged), "/source", "copy must not run for a legacy-seeded volume") -} - func TestLocalDockerDelivery_Seed_CopyAndCleanupFailureJoined(t *testing.T) { tmpDir := t.TempDir() diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index 29eadb4ab..a80f09397 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -11,11 +11,6 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) -// legacySeededSentinel is the pre-label marker file older devsy versions wrote -// into a seeded volume. Retained only to recognize volumes seeded before the -// seeded label existed. -const legacySeededSentinel = ".devsy-seeded" - // WorkspaceSeedOptions describes a request to seed a named workspace volume // from a local source directory. type WorkspaceSeedOptions struct { @@ -138,28 +133,7 @@ func (d *LocalDockerDelivery) volumeSeedState( return false, false, fmt.Errorf("inspect volume %s: %s: %w", name, string(out), err) } managedStr, seededStr, _ := strings.Cut(strings.TrimSpace(string(out)), ",") - managed = managedStr == pkgconfig.LabelValueTrue - seeded = seededStr == pkgconfig.LabelValueTrue - - // Volumes seeded by older versions predate the seeded label and are only - // marked by a sentinel file; treat that as seeded so they are not - // re-seeded over user changes. - if managed && !seeded && d.volumeHasLegacySentinel(ctx, name) { - seeded = true - } - return managed, seeded, nil -} - -// volumeHasLegacySentinel reports whether the pre-label seeded marker file is -// present in the volume. -func (d *LocalDockerDelivery) volumeHasLegacySentinel(ctx context.Context, name string) bool { - args := []string{ - cmdRun, flagRM, - "-v", name + ":/target:ro", - d.helperImageName(), - "sh", "-c", "[ -e /target/" + legacySeededSentinel + " ]", - } - return d.cmd(ctx, args...).Run() == nil + return managedStr == pkgconfig.LabelValueTrue, seededStr == pkgconfig.LabelValueTrue, nil } func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) bool { From fe87390875b2c56268db8b3c8673cc846c95b0a6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 7 Jul 2026 14:59:28 -0500 Subject: [PATCH 9/9] chore: remove comments --- pkg/agent/delivery/local_docker_test.go | 2 -- pkg/agent/delivery/workspace_seed.go | 5 ----- pkg/devcontainer/config/artifacts.go | 3 --- 3 files changed, 10 deletions(-) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index c7972840a..6f512e595 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -453,8 +453,6 @@ func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { func TestLocalDockerDelivery_Seed_CopyAndCleanupFailureJoined(t *testing.T) { tmpDir := t.TempDir() - // Fresh volume (inspect fails), copy fails, and cleanup (volume rm) also - // fails: both errors must surface rather than be swallowed. scriptPath := filepath.Join(tmpDir, "fake-docker.sh") script := "#!/bin/sh\n" + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 1; fi\n" + diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index a80f09397..953a08ebf 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -60,9 +60,6 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( } if err := d.copyDirIntoVolume(ctx, opts.SourceDir, opts.VolumeName); err != nil { - // The volume is now labeled seeded but holds partial contents. Removing - // it lets the next up re-seed; if removal also fails, surface both so a - // partial volume is never silently treated as complete. if rmErr := d.removeVolume(ctx, opts.VolumeName); rmErr != nil { return errors.Join( fmt.Errorf("seed workspace volume: %w", err), @@ -141,8 +138,6 @@ func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) boo return err == nil } -// copyDirIntoVolume copies sourceDir into the volume root via a throwaway -// helper container, excluding devsy's build artifacts. func (d *LocalDockerDelivery) copyDirIntoVolume( ctx context.Context, sourceDir, volumeName string, diff --git a/pkg/devcontainer/config/artifacts.go b/pkg/devcontainer/config/artifacts.go index 244e2ec1c..cae41b5b7 100644 --- a/pkg/devcontainer/config/artifacts.go +++ b/pkg/devcontainer/config/artifacts.go @@ -11,13 +11,10 @@ var buildArtifactNames = []string{ DevsyContextFeatureFolder, } -// BuildArtifactExcludes returns the build artifact names to exclude when -// copying, streaming, or hashing a workspace tree. func BuildArtifactExcludes() []string { return append([]string(nil), buildArtifactNames...) } -// RemoveBuildArtifacts deletes devsy's build artifacts from contextPath. func RemoveBuildArtifacts(contextPath string) { for _, name := range buildArtifactNames { path := filepath.Join(contextPath, name)