From cd0c96fa66dcfeac2324d0b97431082c225c32af Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 3 May 2026 09:13:42 -0500 Subject: [PATCH 1/3] fix(lifecycle): run postAttachCommand on every attach per spec --- cmd/agent/container/setup.go | 2 +- e2e/tests/up/provider_docker.go | 39 +++++++ .../.devcontainer.json | 4 + pkg/command/background.go | 36 ++++-- pkg/command/background_test.go | 42 +++++++ pkg/devcontainer/setup/lifecyclehooks_test.go | 104 ++++++++++++++++++ 6 files changed, 217 insertions(+), 10 deletions(-) create mode 100644 e2e/tests/up/testdata/docker-post-attach-every-time/.devcontainer.json create mode 100644 pkg/command/background_test.go diff --git a/cmd/agent/container/setup.go b/cmd/agent/container/setup.go index 31240384c..181dce551 100644 --- a/cmd/agent/container/setup.go +++ b/cmd/agent/container/setup.go @@ -476,7 +476,7 @@ func (cmd *SetupContainerCmd) startPostAttachHooks(sctx *setupContext) error { return nil } - return command.StartBackgroundOnce("devsy.post-attach", func() (*exec.Cmd, error) { + return command.StartBackground("devsy.post-attach", func() (*exec.Cmd, error) { log.Debugf("starting postAttachCommand as background process") binaryPath, err := os.Executable() if err != nil { diff --git a/e2e/tests/up/provider_docker.go b/e2e/tests/up/provider_docker.go index 7b1b93773..1092fa266 100644 --- a/e2e/tests/up/provider_docker.go +++ b/e2e/tests/up/provider_docker.go @@ -467,6 +467,45 @@ var _ = ginkgo.Describe( ) }, ginkgo.SpecTimeout(framework.TimeoutShort())) + ginkgo.It("postAttachCommand runs on every attach", func(ctx context.Context) { + tempDir, err := setupWorkspace( + "tests/up/testdata/docker-post-attach-every-time", + dtc.initialDir, + dtc.f, + ) + framework.ExpectNoError(err) + + // First attach + err = dtc.f.DevsyUp(ctx, tempDir) + framework.ExpectNoError(err) + + gomega.Eventually(func() string { + out, err := dtc.execSSH(ctx, tempDir, "cat $HOME/attach-count.out 2>/dev/null") + if err != nil { + return "" + } + return strings.TrimSpace(out) + }).WithTimeout(15*time.Second).WithPolling(1*time.Second).Should( + gomega.Equal("1"), + "postAttachCommand should run on first attach", + ) + + // Second attach (re-up the same workspace) + err = dtc.f.DevsyUp(ctx, tempDir) + framework.ExpectNoError(err) + + gomega.Eventually(func() string { + out, err := dtc.execSSH(ctx, tempDir, "cat $HOME/attach-count.out 2>/dev/null") + if err != nil { + return "" + } + return strings.TrimSpace(out) + }).WithTimeout(15*time.Second).WithPolling(1*time.Second).Should( + gomega.Equal("2"), + "postAttachCommand should run again on second attach", + ) + }, ginkgo.SpecTimeout(framework.TimeoutShort())) + ginkgo.It( "initializeCommand with object syntax runs named sub-commands in parallel", func(ctx context.Context) { diff --git a/e2e/tests/up/testdata/docker-post-attach-every-time/.devcontainer.json b/e2e/tests/up/testdata/docker-post-attach-every-time/.devcontainer.json new file mode 100644 index 000000000..fda6a67f4 --- /dev/null +++ b/e2e/tests/up/testdata/docker-post-attach-every-time/.devcontainer.json @@ -0,0 +1,4 @@ +{ + "image": "ghcr.io/devsy-org/test-images/go:1", + "postAttachCommand": "count=$(cat $HOME/attach-count.out 2>/dev/null || echo 0); echo $((count+1)) > $HOME/attach-count.out" +} diff --git a/pkg/command/background.go b/pkg/command/background.go index 924ba0740..b97d9a109 100644 --- a/pkg/command/background.go +++ b/pkg/command/background.go @@ -65,7 +65,28 @@ func StartBackgroundOnce(commandName string, createCommand CreateCommand) error return startCommand(cmd, pidFile, streamsFile) } +// StartBackground starts a background process unconditionally, without any +// once-guard. Use this for commands that must run on every invocation (e.g. +// postAttachCommand which runs on every attach per the devcontainer spec). +func StartBackground(commandName string, createCommand CreateCommand) error { + streamsFile, err := config.DefaultPathManager().ProcessStreamsFile(commandName) + if err != nil { + return fmt.Errorf("process streams file: %w", err) + } + + cmd, err := createCommand() + if err != nil { + return err + } + + return startDetached(cmd, "", streamsFile) +} + func startCommand(cmd *exec.Cmd, pidFile, streamsFile string) error { + return startDetached(cmd, pidFile, streamsFile) +} + +func startDetached(cmd *exec.Cmd, pidFile, streamsFile string) error { streamsF, err := openStreamsFile(cmd, streamsFile) if err != nil { return err @@ -75,19 +96,16 @@ func startCommand(cmd *exec.Cmd, pidFile, streamsFile string) error { closeFile(streamsF) return fmt.Errorf("start process: %w", err) } - // Close the parent's copy of the streams fd. After Start() forks, the - // child has its own copy; the parent no longer needs it. closeFile(streamsF) - if err := os.WriteFile(pidFile, []byte(strconv.Itoa(cmd.Process.Pid)), 0o600); err != nil { - // Process is running but untracked. Kill and reap it to prevent orphans/zombies. - _ = cmd.Process.Kill() - _ = cmd.Wait() - return fmt.Errorf("write pid file (process killed to prevent orphan): %w", err) + if pidFile != "" { + if err := os.WriteFile(pidFile, []byte(strconv.Itoa(cmd.Process.Pid)), 0o600); err != nil { + _ = cmd.Process.Kill() + _ = cmd.Wait() + return fmt.Errorf("write pid file (process killed to prevent orphan): %w", err) + } } - // Release the process handle so the child runs independently of this - // parent process. After release, we cannot wait for or signal it. _ = cmd.Process.Release() return nil diff --git a/pkg/command/background_test.go b/pkg/command/background_test.go new file mode 100644 index 000000000..3b88a7b38 --- /dev/null +++ b/pkg/command/background_test.go @@ -0,0 +1,42 @@ +package command + +import ( + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestStartBackgroundRunsEveryTime(t *testing.T) { + callCount := 0 + createCmd := func() (*exec.Cmd, error) { + callCount++ + return exec.Command("true"), nil + } + + err := StartBackground("test.start-bg", createCmd) + require.NoError(t, err) + assert.Equal(t, 1, callCount) + + err = StartBackground("test.start-bg", createCmd) + require.NoError(t, err) + assert.Equal(t, 2, callCount, "StartBackground must invoke createCommand on every call") +} + +func TestStartBackgroundOnceSkipsSecondCall(t *testing.T) { + callCount := 0 + createCmd := func() (*exec.Cmd, error) { + callCount++ + // Use sleep so the process is still running when the second call happens. + return exec.Command("sleep", "5"), nil + } + + err := StartBackgroundOnce("test.start-bg-once", createCmd) + require.NoError(t, err) + assert.Equal(t, 1, callCount) + + err = StartBackgroundOnce("test.start-bg-once", createCmd) + require.NoError(t, err) + assert.Equal(t, 1, callCount, "StartBackgroundOnce must skip when process is already running") +} diff --git a/pkg/devcontainer/setup/lifecyclehooks_test.go b/pkg/devcontainer/setup/lifecyclehooks_test.go index 8a9bc7f84..0ebb69c71 100644 --- a/pkg/devcontainer/setup/lifecyclehooks_test.go +++ b/pkg/devcontainer/setup/lifecyclehooks_test.go @@ -108,6 +108,7 @@ func (s *LifecycleHookTestSuite) TestLifecycleHooksNoOpWithEmptyConfig() { assert.NoError(s.T(), err) } + func (s *LifecycleHookTestSuite) TestResolveLifecycleEnvIncludesSecrets() { t := s.T() @@ -678,6 +679,109 @@ func (s *LifecycleHookTestSuite) TestMergeSecretsEnvValueWithEquals() { assert.Equal(s.T(), "host=db port=5432", env["CONN"]) } +func (s *LifecycleHookTestSuite) TestPostAttachHooksRunEveryTime() { + t := s.T() + currentUser, err := user.Current() + assert.NoError(t, err) + + dir := t.TempDir() + counterFile := filepath.Join(dir, "counter.txt") + + result := &config.Result{ + MergedConfig: &config.MergedDevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + RemoteUser: currentUser.Username, + }, + UpdatedConfigProperties: config.UpdatedConfigProperties{ + PostAttachCommands: []types.LifecycleHook{ + {"": {"sh", "-c", fmt.Sprintf( + `count=$(cat %s 2>/dev/null || echo 0); echo $((count+1)) > %s`, + counterFile, counterFile, + )}}, + }, + }, + }, + ContainerDetails: &config.ContainerDetails{ + State: config.ContainerDetailsState{}, + }, + SubstitutionContext: &config.SubstitutionContext{ + ContainerWorkspaceFolder: dir, + }, + } + + // Run postAttachCommand multiple times — it must execute every time. + for i := 1; i <= 3; i++ { + err := RunPostAttachHooks(context.Background(), result, nil) + assert.NoError(t, err) + + content, readErr := os.ReadFile(counterFile) //nolint:gosec // test file from TempDir + assert.NoError(t, readErr) + assert.Equal(t, fmt.Sprintf("%d\n", i), string(content), + "postAttachCommand should run on call %d", i) + } +} + +func (s *LifecycleHookTestSuite) TestPostCreateHookUsesOnceSemantics() { + t := s.T() + + // postCreateCommand passes container.Created as content to shouldSkipHook. + // When content is non-empty, shouldSkipHook uses a marker file to ensure + // the hook runs only once per container creation. + skip, err := shouldSkipHook("test-postCreate", "") + assert.NoError(t, err) + assert.False(t, skip, "empty content should never skip") + + // Verify that preAttachPhaseParams sets content for postCreate and postStart. + env := lifecycleEnv{remoteUser: "test", workspaceFolder: "/tmp"} + result := &config.Result{ + MergedConfig: &config.MergedDevContainerConfig{ + UpdatedConfigProperties: config.UpdatedConfigProperties{ + PostCreateCommands: []types.LifecycleHook{{"": {"echo", "hi"}}}, + PostStartCommands: []types.LifecycleHook{{"": {"echo", "hi"}}}, + }, + }, + ContainerDetails: &config.ContainerDetails{ + Created: "2024-01-01T00:00:00Z", + State: config.ContainerDetailsState{StartedAt: "2024-01-01T00:00:01Z"}, + }, + SubstitutionContext: &config.SubstitutionContext{ContainerWorkspaceFolder: "/tmp"}, + } + + hooks := preAttachPhaseParams(result, env, false) + + // postCreateCommand should have content = Created (non-empty → once semantics). + var postCreate, postStart hookRunParams + for _, h := range hooks { + if h.phase == PhasePostCreate { + postCreate = h.params + } + if h.phase == PhasePostStart { + postStart = h.params + } + } + assert.NotEmpty(t, postCreate.content, + "postCreateCommand must have non-empty content for once-semantics") + assert.NotEmpty(t, postStart.content, + "postStartCommand must have non-empty content for once-semantics") +} + +func (s *LifecycleHookTestSuite) TestPostAttachHookHasNoOnceGuard() { + t := s.T() + + // RunPostAttachHooks passes content="" which means shouldSkipHook always + // returns false — the hook runs every time. Verify the contract. + skip, err := shouldSkipHook("postAttachCommands", "") + assert.NoError(t, err) + assert.False(t, skip, "postAttachCommand must never be skipped (content is always empty)") + + // Call shouldSkipHook multiple times with empty content — must never skip. + for i := 0; i < 5; i++ { + skip, err := shouldSkipHook("postAttachCommands", "") + assert.NoError(t, err) + assert.False(t, skip, "postAttachCommand must not skip on call %d", i) + } +} + func TestLifecycleHookTestSuite(t *testing.T) { suite.Run(t, new(LifecycleHookTestSuite)) } From 8cb1267238f18f23c1e06473164ddfc9ff728fc7 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 3 May 2026 11:37:57 -0500 Subject: [PATCH 2/3] fix(lint): modernize range-over-int in lifecyclehooks_test --- pkg/devcontainer/setup/lifecyclehooks_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/devcontainer/setup/lifecyclehooks_test.go b/pkg/devcontainer/setup/lifecyclehooks_test.go index 0ebb69c71..e27b8b5da 100644 --- a/pkg/devcontainer/setup/lifecyclehooks_test.go +++ b/pkg/devcontainer/setup/lifecyclehooks_test.go @@ -108,7 +108,6 @@ func (s *LifecycleHookTestSuite) TestLifecycleHooksNoOpWithEmptyConfig() { assert.NoError(s.T(), err) } - func (s *LifecycleHookTestSuite) TestResolveLifecycleEnvIncludesSecrets() { t := s.T() @@ -775,7 +774,7 @@ func (s *LifecycleHookTestSuite) TestPostAttachHookHasNoOnceGuard() { assert.False(t, skip, "postAttachCommand must never be skipped (content is always empty)") // Call shouldSkipHook multiple times with empty content — must never skip. - for i := 0; i < 5; i++ { + for i := range 5 { skip, err := shouldSkipHook("postAttachCommands", "") assert.NoError(t, err) assert.False(t, skip, "postAttachCommand must not skip on call %d", i) From 9d42110c3502096526ef2f589199954cb9a093f2 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 3 May 2026 11:38:31 -0500 Subject: [PATCH 3/3] fix(test): use SetPathManager for platform-independent test isolation --- pkg/command/background_test.go | 35 +++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/pkg/command/background_test.go b/pkg/command/background_test.go index 3b88a7b38..6ca9351ab 100644 --- a/pkg/command/background_test.go +++ b/pkg/command/background_test.go @@ -1,42 +1,67 @@ package command import ( + "fmt" "os/exec" "testing" + "time" + "github.com/devsy-org/devsy/pkg/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +type testPathManager struct { + config.PathManager + runtimeDir string +} + +func (t *testPathManager) RuntimeDir() (string, error) { return t.runtimeDir, nil } + func TestStartBackgroundRunsEveryTime(t *testing.T) { + tmpDir := t.TempDir() + config.SetPathManager( + &testPathManager{PathManager: config.NewPathManager(), runtimeDir: tmpDir}, + ) + t.Cleanup(func() { config.ResetPathManager() }) + + commandName := fmt.Sprintf("test.start-bg.%d", time.Now().UnixNano()) + callCount := 0 createCmd := func() (*exec.Cmd, error) { callCount++ return exec.Command("true"), nil } - err := StartBackground("test.start-bg", createCmd) + err := StartBackground(commandName, createCmd) require.NoError(t, err) assert.Equal(t, 1, callCount) - err = StartBackground("test.start-bg", createCmd) + err = StartBackground(commandName, createCmd) require.NoError(t, err) assert.Equal(t, 2, callCount, "StartBackground must invoke createCommand on every call") } func TestStartBackgroundOnceSkipsSecondCall(t *testing.T) { + tmpDir := t.TempDir() + config.SetPathManager( + &testPathManager{PathManager: config.NewPathManager(), runtimeDir: tmpDir}, + ) + t.Cleanup(func() { config.ResetPathManager() }) + + commandName := fmt.Sprintf("test.start-bg-once.%d", time.Now().UnixNano()) + callCount := 0 createCmd := func() (*exec.Cmd, error) { callCount++ - // Use sleep so the process is still running when the second call happens. return exec.Command("sleep", "5"), nil } - err := StartBackgroundOnce("test.start-bg-once", createCmd) + err := StartBackgroundOnce(commandName, createCmd) require.NoError(t, err) assert.Equal(t, 1, callCount) - err = StartBackgroundOnce("test.start-bg-once", createCmd) + err = StartBackgroundOnce(commandName, createCmd) require.NoError(t, err) assert.Equal(t, 1, callCount, "StartBackgroundOnce must skip when process is already running") }