Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion cmd/internal/agent_daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func (cmd *DaemonCmd) checkAndShutdown(
latestActivity time.Time,
workspace *provider2.AgentWorkspaceInfo,
) {
if cmd.ShutdownAction == config.ShutdownActionNone {
if cmd.effectiveShutdownAction(workspace) == config.ShutdownActionNone {
return
}

Expand Down Expand Up @@ -201,6 +201,20 @@ func (cmd *DaemonCmd) checkAndShutdown(
cmd.runShutdownCommand(ctx, workspace)
}

// effectiveShutdownAction prefers the workspace's resolved config, falling back
// to the daemon's install-time flag when the workspace has none yet.
func (cmd *DaemonCmd) effectiveShutdownAction(
workspace *provider2.AgentWorkspaceInfo,
) string {
if workspace != nil &&
workspace.LastDevContainerConfig != nil &&
workspace.LastDevContainerConfig.Config != nil &&
workspace.LastDevContainerConfig.Config.ShutdownAction != "" {
return workspace.LastDevContainerConfig.Config.ShutdownAction
}
return cmd.ShutdownAction
}

func (cmd *DaemonCmd) runShutdownCommand(
ctx context.Context,
workspace *provider2.AgentWorkspaceInfo,
Expand Down
43 changes: 43 additions & 0 deletions cmd/internal/agent_daemon_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"github.com/devsy-org/devsy/pkg/agent"
"github.com/devsy-org/devsy/pkg/devcontainer/config"
provider2 "github.com/devsy-org/devsy/pkg/provider"
"github.com/devsy-org/devsy/pkg/types"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -89,6 +90,48 @@ func TestFindLatestActivity_PicksLatest(t *testing.T) {
assert.Equal(t, recentTime, *activity)
}

func workspaceWithShutdownAction(action string) *provider2.AgentWorkspaceInfo {
ws := &provider2.AgentWorkspaceInfo{Workspace: &provider2.Workspace{ID: "ws-test"}}
if action != "" {
ws.LastDevContainerConfig = &config.DevContainerConfigWithPath{
Config: &config.DevContainerConfig{
DevContainerConfigBase: config.DevContainerConfigBase{ShutdownAction: action},
},
}
}
return ws
}

func TestEffectiveShutdownAction(t *testing.T) {
t.Run("prefers per-workspace config over flag", func(t *testing.T) {
cmd := &DaemonCmd{ShutdownAction: config.ShutdownActionStopContainer}
ws := workspaceWithShutdownAction(config.ShutdownActionNone)
assert.Equal(t, config.ShutdownActionNone, cmd.effectiveShutdownAction(ws))
})

t.Run("falls back to flag when workspace has no config", func(t *testing.T) {
cmd := &DaemonCmd{ShutdownAction: config.ShutdownActionNone}
ws := workspaceWithShutdownAction("")
assert.Equal(t, config.ShutdownActionNone, cmd.effectiveShutdownAction(ws))
})

t.Run("falls back to flag when config action is empty", func(t *testing.T) {
cmd := &DaemonCmd{ShutdownAction: config.ShutdownActionNone}
ws := &provider2.AgentWorkspaceInfo{
Workspace: &provider2.Workspace{ID: "ws-test"},
LastDevContainerConfig: &config.DevContainerConfigWithPath{
Config: &config.DevContainerConfig{},
},
}
assert.Equal(t, config.ShutdownActionNone, cmd.effectiveShutdownAction(ws))
})

t.Run("falls back to flag for nil workspace", func(t *testing.T) {
cmd := &DaemonCmd{ShutdownAction: config.ShutdownActionStopContainer}
assert.Equal(t, config.ShutdownActionStopContainer, cmd.effectiveShutdownAction(nil))
})
}

func TestEffectiveActivity(t *testing.T) {
orig := activityFilePath
t.Cleanup(func() { activityFilePath = orig })
Expand Down
18 changes: 18 additions & 0 deletions cmd/internal/agentworkspace/up.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ func (cmd *UpCmd) up(
return err
}

// Persist so the daemon, started before the build resolved the config, can
// read the workspace's shutdownAction on the first up.
persistResolvedConfig(workspaceInfo, result)

// runner.Up can return (result, nil) where result carries a structured
// Error forwarded from the inner container-setup step. Treat that as a
// failure so the agent process exits non-zero and the host doesn't try
Expand Down Expand Up @@ -747,6 +751,20 @@ func installDaemon(workspaceInfo *provider.AgentWorkspaceInfo) error {
)
}

func persistResolvedConfig(
workspaceInfo *provider.AgentWorkspaceInfo,
result *config2.Result,
) {
if result == nil || result.Error != "" || result.DevContainerConfigWithPath == nil {
return
}

workspaceInfo.LastDevContainerConfig = result.DevContainerConfigWithPath
if err := agent.PersistAgentWorkspaceInfo(workspaceInfo); err != nil {
log.Errorf("persist resolved devcontainer config: %v", err)
}
Comment on lines +754 to +765

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Do not silently accept failed shutdown-policy persistence.

When PersistAgentWorkspaceInfo fails, this helper only logs and the caller still forwards a successful startup result. The daemon can then fall back to a stale or install-time action, reintroducing the shutdownAction: none failure when workspace.json was not updated. Retry or propagate this failure before reporting startup success, and test the failure path.

🤖 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 `@cmd/internal/agentworkspace/up.go` around lines 754 - 765, Update
persistResolvedConfig and its startup caller so PersistAgentWorkspaceInfo
failures are propagated or retried and prevent a successful startup result from
being forwarded. Preserve the existing early returns and successful persistence
behavior, and add coverage for the failed-persistence path to ensure stale
shutdown policy is not accepted.

}

func downloadLocalFolder(
ctx context.Context,
workspaceDir string,
Expand Down
14 changes: 13 additions & 1 deletion pkg/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,18 @@ func DeleteWorkspaceBusyFile(folder string) {
_ = os.Remove(filepath.Join(folder, config.WorkspaceBusyFile))
}

// PersistAgentWorkspaceInfo writes the workspace info back to workspace.json
// under workspaceInfo.Origin.
func PersistAgentWorkspaceInfo(workspaceInfo *provider2.AgentWorkspaceInfo) error {
if workspaceInfo == nil || workspaceInfo.Origin == "" {
return errors.New("workspace origin is not set")
}
return writeWorkspaceInfo(
filepath.Join(workspaceInfo.Origin, provider2.WorkspaceConfigFile),
workspaceInfo,
)
}

func writeWorkspaceInfo(file string, workspaceInfo *provider2.AgentWorkspaceInfo) error {
// copy workspace info
cloned := provider2.CloneAgentWorkspaceInfo(workspaceInfo)
Expand All @@ -327,7 +339,7 @@ func writeWorkspaceInfo(file string, workspaceInfo *provider2.AgentWorkspaceInfo
cloned.CLIOptions = provider2.CLIOptions{}

// encode workspace info
encoded, err := json.Marshal(workspaceInfo)
encoded, err := json.Marshal(cloned)
if err != nil {
return err
}
Expand Down
56 changes: 56 additions & 0 deletions pkg/agent/persist_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package agent

import (
"path/filepath"
"testing"

"github.com/devsy-org/devsy/pkg/devcontainer/config"
provider2 "github.com/devsy-org/devsy/pkg/provider"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestPersistAgentWorkspaceInfo_RoundTripsResolvedConfig(t *testing.T) {
dir := t.TempDir()
info := &provider2.AgentWorkspaceInfo{
Origin: dir,
Workspace: &provider2.Workspace{ID: "ws-test"},
LastDevContainerConfig: &config.DevContainerConfigWithPath{
Config: &config.DevContainerConfig{
DevContainerConfigBase: config.DevContainerConfigBase{
ShutdownAction: config.ShutdownActionNone,
},
},
},
}

require.NoError(t, PersistAgentWorkspaceInfo(info))

got, err := ParseAgentWorkspaceInfo(filepath.Join(dir, provider2.WorkspaceConfigFile))
require.NoError(t, err)
require.NotNil(t, got.LastDevContainerConfig)
require.NotNil(t, got.LastDevContainerConfig.Config)
assert.Equal(t, config.ShutdownActionNone, got.LastDevContainerConfig.Config.ShutdownAction)
}

func TestPersistAgentWorkspaceInfo_RequiresOrigin(t *testing.T) {
assert.Error(t, PersistAgentWorkspaceInfo(&provider2.AgentWorkspaceInfo{}))
assert.Error(t, PersistAgentWorkspaceInfo(nil))
}

func TestPersistAgentWorkspaceInfo_DoesNotPersistCLIOptions(t *testing.T) {
dir := t.TempDir()
info := &provider2.AgentWorkspaceInfo{
Origin: dir,
Workspace: &provider2.Workspace{ID: "ws-test"},
CLIOptions: provider2.CLIOptions{DaemonInterval: "3s"},
}

require.NoError(t, PersistAgentWorkspaceInfo(info))

got, err := ParseAgentWorkspaceInfo(filepath.Join(dir, provider2.WorkspaceConfigFile))
require.NoError(t, err)
assert.Equal(t, "ws-test", got.Workspace.ID)
assert.Empty(t, got.CLIOptions.DaemonInterval, "CLIOptions must not be persisted")
assert.Equal(t, "3s", info.CLIOptions.DaemonInterval, "caller's struct must not be mutated")
}
Loading