From 5c36b3e4a0b83f4d0d08e48aa438ff82c17421ec Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 27 Jul 2026 00:00:09 -0500 Subject: [PATCH] fix(ci): wait for postCreateCommand before running the command devsy ci ran the user command immediately after RunHeadless returned, without waiting for deferred lifecycle hooks (postCreateCommand) to finish. With the default waitFor=updateContentCommand, postCreate runs in a detached background process, so the command could execute before the container was fully provisioned (e.g. secrets injected by a postCreateCommand were missing). ci now floors the effective waitFor at postCreateCommand so provisioning hooks run synchronously. The floor never lowers a later waitFor a user configured, per the devcontainer spec. postStartCommand stays deferred to avoid blocking ci on long-running start commands. Collapse RunPreAttachHooks parameters into a PreAttachOptions struct. --- cmd/ci/ci.go | 3 ++ cmd/internal/agentcontainer/deferred_hooks.go | 16 +++--- cmd/internal/agentcontainer/setup.go | 1 + pkg/devcontainer/setup/lifecyclehooks.go | 54 +++++++++++-------- pkg/devcontainer/setup/lifecyclehooks_test.go | 32 ++++++----- pkg/devcontainer/setup/setup.go | 18 +++---- pkg/provider/workspace.go | 2 + 7 files changed, 76 insertions(+), 50 deletions(-) diff --git a/cmd/ci/ci.go b/cmd/ci/ci.go index 7a1bd04d4..afcf8d518 100644 --- a/cmd/ci/ci.go +++ b/cmd/ci/ci.go @@ -13,6 +13,7 @@ import ( "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer" + "github.com/devsy-org/devsy/pkg/devcontainer/setup" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" @@ -199,6 +200,8 @@ func (cmd *CICmd) execute(ctx context.Context, source string) (err error) { return fmt.Errorf("ci is currently not supported for proxy providers") } + cmd.WaitFor = string(setup.PhasePostCreate) + if _, err := up.RunHeadless(ctx, workspaceClient, up.HeadlessOptions{ GlobalFlags: cmd.GlobalFlags, DevsyConfig: devsyConfig, diff --git a/cmd/internal/agentcontainer/deferred_hooks.go b/cmd/internal/agentcontainer/deferred_hooks.go index ef8ba804f..8e4c2f095 100644 --- a/cmd/internal/agentcontainer/deferred_hooks.go +++ b/cmd/internal/agentcontainer/deferred_hooks.go @@ -113,12 +113,16 @@ func (cmd *DeferredHooksCmd) Run(ctx context.Context) error { } log.Debugf("running deferred lifecycle hooks") - deferred, err := setup.RunPreAttachHooks(ctx, setupInfo, cmd.Prebuild, setup.DotfilesConfig{ - Repository: cmd.DotfilesRepo, - InstallScript: cmd.DotfilesScript, - RemoteUser: config.GetRemoteUser(setupInfo), - // Mount values aren't in this process's env; read them back for redaction. - }, secretsEnvFromEnvironment(), setup.MountSecretsForRedaction(), setup.SkipPhases{}) + deferred, err := setup.RunPreAttachHooks(ctx, setupInfo, setup.PreAttachOptions{ + Prebuild: cmd.Prebuild, + Dotfiles: setup.DotfilesConfig{ + Repository: cmd.DotfilesRepo, + InstallScript: cmd.DotfilesScript, + RemoteUser: config.GetRemoteUser(setupInfo), + }, + SecretsEnv: secretsEnvFromEnvironment(), + SecretsMount: setup.MountSecretsForRedaction(), + }) if err != nil { return fmt.Errorf("deferred hooks setup: %w", err) } diff --git a/cmd/internal/agentcontainer/setup.go b/cmd/internal/agentcontainer/setup.go index 211991729..386b3562d 100644 --- a/cmd/internal/agentcontainer/setup.go +++ b/cmd/internal/agentcontainer/setup.go @@ -254,6 +254,7 @@ func (cmd *SetupContainerCmd) finalizeSetup(sctx *setupContext) error { SkipPostCreate: sctx.workspaceInfo.CLIOptions.SkipPostCreate, SkipPostStart: sctx.workspaceInfo.CLIOptions.SkipPostStart, SkipPostAttach: sctx.workspaceInfo.CLIOptions.SkipPostAttach, + WaitFor: setup.LifecyclePhase(sctx.workspaceInfo.CLIOptions.WaitFor), Dotfiles: setup.DotfilesConfig{ Repository: cmd.DotfilesRepo, InstallScript: cmd.DotfilesScript, diff --git a/pkg/devcontainer/setup/lifecyclehooks.go b/pkg/devcontainer/setup/lifecyclehooks.go index 3d6a06421..752f9a423 100644 --- a/pkg/devcontainer/setup/lifecyclehooks.go +++ b/pkg/devcontainer/setup/lifecyclehooks.go @@ -72,17 +72,19 @@ func phaseHasCommands(all []phaseHook, phase LifecyclePhase) bool { return false } -// resolveWaitFor normalises the raw waitFor string from the config, -// falling back to the spec default for empty or invalid values. -func resolveWaitFor(raw string) LifecyclePhase { - if raw == "" { - return DefaultWaitFor +// resolveWaitFor normalises the raw waitFor string from the config, falling +// back to the spec default for empty or invalid values. A valid floor raises +// the effective phase but never lowers it, so a later waitFor the user +// configured is always honoured. +func resolveWaitFor(raw string, floor LifecyclePhase) LifecyclePhase { + waitFor := DefaultWaitFor + if p := LifecyclePhase(raw); validWaitForPhase(p) { + waitFor = p } - p := LifecyclePhase(raw) - if !validWaitForPhase(p) { - return DefaultWaitFor + if validWaitForPhase(floor) && phaseIndex(floor) > phaseIndex(waitFor) { + return floor } - return p + return waitFor } // promoteDotfilesWaitFor ensures that when dotfiles are configured, the @@ -264,39 +266,47 @@ type SkipPhases struct { PostAttach bool } +// PreAttachOptions configures a RunPreAttachHooks invocation. WaitFor is a +// floor: it raises the effective waitFor phase but never lowers the value +// configured in the devcontainer. Empty means use the configured value. +type PreAttachOptions struct { + Prebuild bool + Dotfiles DotfilesConfig + SecretsEnv []string + SecretsMount []string + Skip SkipPhases + WaitFor LifecyclePhase +} + // RunPreAttachHooks runs lifecycle hooks up to and including the waitFor phase // synchronously and returns a slice of deferred phases that should run in the // background. Dotfiles are installed between postCreateCommand and // postStartCommand per the devcontainer spec. // -// When prebuild is true, only onCreateCommand and updateContentCommand are +// When Prebuild is true, only onCreateCommand and updateContentCommand are // executed and waitFor is ignored. func RunPreAttachHooks( ctx context.Context, setupInfo *config.Result, - prebuild bool, - dotfiles DotfilesConfig, - secretsEnv []string, - secretsMount []string, - skip SkipPhases, + opts PreAttachOptions, ) (DeferredHooks, error) { env := resolveLifecycleEnv(ctx, setupInfo) - mergeSecretsEnv(env.remoteEnv, secretsEnv, secretsMount) - all := preAttachPhaseParams(setupInfo, env, prebuild) + mergeSecretsEnv(env.remoteEnv, opts.SecretsEnv, opts.SecretsMount) + all := preAttachPhaseParams(setupInfo, env, opts.Prebuild) // Insert the dotfiles phase between postCreate and postStart. created := setupInfo.ContainerDetails.Created - all = insertDotfilesPhase(ctx, all, dotfiles, created) + all = insertDotfilesPhase(ctx, all, opts.Dotfiles, created) // Remove skipped phases. - all = filterSkippedPhases(all, skip) + all = filterSkippedPhases(all, opts.Skip) - if prebuild { + if opts.Prebuild { return DeferredHooks{}, runPrebuildHooks(all) } - waitFor := resolveWaitFor(setupInfo.MergedConfig.WaitFor) - waitFor = promoteDotfilesWaitFor(waitFor, dotfiles) + waitFor := resolveWaitFor(setupInfo.MergedConfig.WaitFor, opts.WaitFor) + waitFor = promoteDotfilesWaitFor(waitFor, opts.Dotfiles) if !phaseHasCommands(all, waitFor) { log.Debugf( diff --git a/pkg/devcontainer/setup/lifecyclehooks_test.go b/pkg/devcontainer/setup/lifecyclehooks_test.go index 14f18fede..cb5f30c26 100644 --- a/pkg/devcontainer/setup/lifecyclehooks_test.go +++ b/pkg/devcontainer/setup/lifecyclehooks_test.go @@ -99,8 +99,7 @@ func (s *LifecycleHookTestSuite) TestLifecycleHooksNoOpWithEmptyConfig() { }, } - // Both functions should return nil with empty config (no commands to run) - deferred, err := RunPreAttachHooks(ctx, result, false, DotfilesConfig{}, nil, nil, SkipPhases{}) + deferred, err := RunPreAttachHooks(ctx, result, PreAttachOptions{}) assert.NoError(s.T(), err) assert.True(s.T(), deferred.Empty()) @@ -139,21 +138,29 @@ func (s *LifecycleHookTestSuite) TestResolveLifecycleEnvIncludesSecrets() { } func (s *LifecycleHookTestSuite) TestResolveWaitForDefault() { - assert.Equal(s.T(), DefaultWaitFor, resolveWaitFor("")) + assert.Equal(s.T(), DefaultWaitFor, resolveWaitFor("", "")) } func (s *LifecycleHookTestSuite) TestResolveWaitForValid() { - assert.Equal(s.T(), PhasePostCreate, resolveWaitFor("postCreateCommand")) - assert.Equal(s.T(), PhasePostStart, resolveWaitFor("postStartCommand")) - assert.Equal(s.T(), PhaseOnCreate, resolveWaitFor("onCreateCommand")) - assert.Equal(s.T(), PhasePostAttach, resolveWaitFor("postAttachCommand")) - assert.Equal(s.T(), PhaseUpdateContent, resolveWaitFor("updateContentCommand")) - assert.Equal(s.T(), PhaseInitializeCommand, resolveWaitFor("initializeCommand")) + assert.Equal(s.T(), PhasePostCreate, resolveWaitFor("postCreateCommand", "")) + assert.Equal(s.T(), PhasePostStart, resolveWaitFor("postStartCommand", "")) + assert.Equal(s.T(), PhaseOnCreate, resolveWaitFor("onCreateCommand", "")) + assert.Equal(s.T(), PhasePostAttach, resolveWaitFor("postAttachCommand", "")) + assert.Equal(s.T(), PhaseUpdateContent, resolveWaitFor("updateContentCommand", "")) + assert.Equal(s.T(), PhaseInitializeCommand, resolveWaitFor("initializeCommand", "")) } func (s *LifecycleHookTestSuite) TestResolveWaitForInvalid() { - assert.Equal(s.T(), DefaultWaitFor, resolveWaitFor("bogus")) - assert.Equal(s.T(), DefaultWaitFor, resolveWaitFor("POSTCREATECOMMAND")) + assert.Equal(s.T(), DefaultWaitFor, resolveWaitFor("bogus", "")) + assert.Equal(s.T(), DefaultWaitFor, resolveWaitFor("POSTCREATECOMMAND", "")) +} + +func (s *LifecycleHookTestSuite) TestResolveWaitForFloor() { + assert.Equal(s.T(), PhasePostCreate, resolveWaitFor("updateContentCommand", PhasePostCreate)) + assert.Equal(s.T(), PhasePostStart, resolveWaitFor("postStartCommand", PhasePostCreate)) + assert.Equal(s.T(), PhasePostCreate, resolveWaitFor("initializeCommand", PhasePostCreate)) + assert.Equal(s.T(), PhaseUpdateContent, resolveWaitFor("updateContentCommand", "")) + assert.Equal(s.T(), PhaseUpdateContent, resolveWaitFor("updateContentCommand", "bogus")) } func (s *LifecycleHookTestSuite) TestRunWithWaitForDefaultSplit() { @@ -251,8 +258,7 @@ func (s *LifecycleHookTestSuite) TestPrebuildIgnoresWaitFor() { }, } - // In prebuild mode, no deferred hooks are returned regardless of waitFor. - deferred, err := RunPreAttachHooks(ctx, result, true, DotfilesConfig{}, nil, nil, SkipPhases{}) + deferred, err := RunPreAttachHooks(ctx, result, PreAttachOptions{Prebuild: true}) assert.NoError(s.T(), err) assert.True(s.T(), deferred.Empty()) } diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index 785c3557d..502f93d5b 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -48,6 +48,7 @@ type ContainerSetupConfig struct { SkipPostCreate bool SkipPostStart bool SkipPostAttach bool + WaitFor LifecyclePhase } // SetupContainerPreAttach runs container setup up to and including the waitFor @@ -78,19 +79,18 @@ func SetupContainerPreAttach( setupOptionalFeatures(ctx, cfg) log.Debugf("running pre-attach lifecycle hooks") - deferred, err := RunPreAttachHooks( - ctx, - cfg.SetupInfo, - cfg.Prebuild, - cfg.Dotfiles, - cfg.SecretsEnv, - cfg.SecretsMount, - SkipPhases{ + deferred, err := RunPreAttachHooks(ctx, cfg.SetupInfo, PreAttachOptions{ + Prebuild: cfg.Prebuild, + Dotfiles: cfg.Dotfiles, + SecretsEnv: cfg.SecretsEnv, + SecretsMount: cfg.SecretsMount, + Skip: SkipPhases{ PostCreate: cfg.SkipPostCreate, PostStart: cfg.SkipPostStart, PostAttach: cfg.SkipPostAttach, }, - ) + WaitFor: cfg.WaitFor, + }) if err != nil { return DeferredHooks{}, fmt.Errorf("lifecycle hooks pre-attach: %w", err) } diff --git a/pkg/provider/workspace.go b/pkg/provider/workspace.go index dfd35cddc..09a939d02 100644 --- a/pkg/provider/workspace.go +++ b/pkg/provider/workspace.go @@ -283,6 +283,8 @@ type CLIOptions struct { SkipPostAttach bool `json:"skipPostAttach,omitempty"` SkipHostRequirements bool `json:"skipHostRequirements,omitempty"` + WaitFor string `json:"waitFor,omitempty"` + // dotfiles options DotfilesRepo string `json:"dotfilesRepo,omitempty"` DotfilesScript string `json:"dotfilesScript,omitempty"`