diff --git a/cmd/internal/agentcontainer/setup.go b/cmd/internal/agentcontainer/setup.go index fae899201..9d82681f9 100644 --- a/cmd/internal/agentcontainer/setup.go +++ b/cmd/internal/agentcontainer/setup.go @@ -108,11 +108,17 @@ func (cmd *SetupContainerCmd) Run(ctx context.Context) error { tunnelClient: tunnelClient, } - if err := cmd.prepareWorkspace(sctx); err != nil { - return err - } - - return cmd.finalizeSetup(sctx) + _, err = tunnelserver.ReportResult( + ctx, + tunnelClient, + func(_ context.Context) (*config.Result, error) { + if err := cmd.prepareWorkspace(sctx); err != nil { + return nil, err + } + return cmd.finalizeSetup(sctx) + }, + ) + return err } func (cmd *SetupContainerCmd) registerFlags(setupContainerCmd *cobra.Command) { @@ -258,10 +264,10 @@ func fetchSecrets( return env, mount, nil } -func (cmd *SetupContainerCmd) finalizeSetup(sctx *setupContext) error { +func (cmd *SetupContainerCmd) finalizeSetup(sctx *setupContext) (*config.Result, error) { secretsEnv, secretsMount, err := fetchSecrets(sctx.ctx, sctx.tunnelClient) if err != nil { - return cmd.reportSetupFailure(sctx, err) + return nil, err } sctx.secretsEnv = secretsEnv @@ -287,31 +293,16 @@ func (cmd *SetupContainerCmd) finalizeSetup(sctx *setupContext) error { deferred, err := setup.SetupContainerPreAttach(sctx.ctx, cfg) if err != nil { - return cmd.reportSetupFailure(sctx, err) + return nil, err } if !cmd.Prebuild { if err := cmd.setupPostAttach(sctx, deferred); err != nil { - return cmd.reportSetupFailure(sctx, err) + return nil, err } } - return cmd.sendSetupResult(sctx.ctx, sctx.setupInfo, sctx.tunnelClient) -} - -// reportSetupFailure forwards a structured error result through the tunnel -// before returning the original error. Without this, the outer agent only -// sees the SSH exit code and the underlying cause (e.g. an IDE install -// failure) gets lost to a generic wrapper on the host side. -func (cmd *SetupContainerCmd) reportSetupFailure(sctx *setupContext, cause error) error { - errResult := &config.Result{Error: cause.Error()} - if sendErr := cmd.sendSetupResult(sctx.ctx, errResult, sctx.tunnelClient); sendErr != nil { - // Failure-on-failure: the host will see only the SSH exit code, so - // log the original cause alongside the send failure to leave a - // breadcrumb for debugging. - log.Errorf("failed to forward setup error %q to host: %v", cause, sendErr) - } - return cause + return sctx.setupInfo, nil } func (cmd *SetupContainerCmd) setupPostAttach( @@ -645,23 +636,6 @@ func (cmd *SetupContainerCmd) startPostAttachHooks(sctx *setupContext) error { }) } -func (cmd *SetupContainerCmd) sendSetupResult( - ctx context.Context, - setupInfo *config.Result, - tunnelClient tunnel.TunnelClient, -) error { - out, err := json.Marshal(setupInfo) - if err != nil { - return fmt.Errorf("marshal setup info: %w", err) - } - - if _, err := tunnelClient.SendResult(ctx, &tunnel.Message{Message: string(out)}); err != nil { - return fmt.Errorf("send result: %w", err) - } - - return nil -} - func fillContainerEnv(setupInfo *config.Result) error { // set remote-env if setupInfo.MergedConfig.RemoteEnv == nil { diff --git a/cmd/internal/agentworkspace/build.go b/cmd/internal/agentworkspace/build.go index 8687e5217..eb18e4c57 100644 --- a/cmd/internal/agentworkspace/build.go +++ b/cmd/internal/agentworkspace/build.go @@ -7,7 +7,9 @@ import ( "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/agent/tunnelserver" "github.com/devsy-org/devsy/pkg/devcontainer" + config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" @@ -65,7 +67,7 @@ func (cmd *BuildCmd) Run(ctx context.Context) error { // initialize the workspace cancelCtx, cancel := context.WithCancel(ctx) defer cancel() - _, credentialsDir, err := initWorkspace(cancelCtx, initWorkspaceParams{ + tunnelClient, credentialsDir, err := initWorkspace(cancelCtx, initWorkspaceParams{ workspaceInfo: workspaceInfo, debug: cmd.Debug, shouldInstallDaemon: false, @@ -83,7 +85,14 @@ func (cmd *BuildCmd) Run(ctx context.Context) error { return err } - return buildAndPushImages(ctx, runner, workspaceInfo) + _, err = tunnelserver.ReportResult( + cancelCtx, + tunnelClient, + func(ctx context.Context) (*config2.Result, error) { + return nil, buildAndPushImages(ctx, runner, workspaceInfo) + }, + ) + return err } func buildAndPushImages( diff --git a/cmd/internal/agentworkspace/up.go b/cmd/internal/agentworkspace/up.go index cd4b15906..c3af1f26c 100644 --- a/cmd/internal/agentworkspace/up.go +++ b/cmd/internal/agentworkspace/up.go @@ -150,53 +150,36 @@ func (cmd *UpCmd) up( workspaceInfo *provider.AgentWorkspaceInfo, tunnelClient tunnel.TunnelClient, ) error { - result, err := cmd.devsyUp(ctx, workspaceInfo, tunnelClient) + result, err := tunnelserver.ReportResult( + ctx, + tunnelClient, + func(ctx context.Context) (*config2.Result, error) { + result, err := cmd.devsyUp(ctx, workspaceInfo, tunnelClient) + if err != nil { + return &config2.Result{ + Error: err.Error(), + RecoveryAvailable: errors.Is(err, clierr.ErrBuildFailedRecoverable), + }, 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) + return result, nil + }, + ) if err != nil { - errResult := &config2.Result{ - Error: err.Error(), - RecoveryAvailable: errors.Is(err, clierr.ErrBuildFailedRecoverable), - } - if sendErr := cmd.sendResult(ctx, errResult, tunnelClient); sendErr != nil { - log.Errorf("failed to forward up error %q to host: %v", err, sendErr) - } 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 // to proceed with a half-populated result. - if err := cmd.sendResult(ctx, result, tunnelClient); err != nil { - return err - } if result != nil && result.Error != "" { return fmt.Errorf("%s", result.Error) } return nil } -func (cmd *UpCmd) sendResult( - ctx context.Context, - result *config2.Result, - tunnelClient tunnel.TunnelClient, -) error { - out, err := json.Marshal(result) - if err != nil { - return err - } - - _, err = tunnelClient.SendResult(ctx, &tunnel.Message{Message: string(out)}) - if err != nil { - return fmt.Errorf("send result: %w", err) - } - - return nil -} - func (cmd *UpCmd) devsyUp( ctx context.Context, workspaceInfo *provider.AgentWorkspaceInfo, diff --git a/pkg/agent/tunnelserver/result_reporter.go b/pkg/agent/tunnelserver/result_reporter.go new file mode 100644 index 000000000..ae2ffd075 --- /dev/null +++ b/pkg/agent/tunnelserver/result_reporter.go @@ -0,0 +1,75 @@ +package tunnelserver + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/devsy-org/devsy/pkg/agent/tunnel" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" +) + +const sendResultTimeout = 5 * time.Second + +// ReportResult runs fn and guarantees its outcome is sent over the tunnel +// via SendResult exactly once before returning, whether fn succeeds or +// fails. +func ReportResult( + ctx context.Context, + tunnelClient tunnel.TunnelClient, + fn func(ctx context.Context) (*config.Result, error), +) (*config.Result, error) { + result, err := runJob(ctx, fn) + + toSend := &config.Result{} + if result != nil { + copied := *result + toSend = &copied + } + if err != nil && toSend.Error == "" { + toSend.Error = err.Error() + } + + if sendErr := sendResult(tunnelClient, toSend); sendErr != nil { + if err != nil { + log.Errorf("failed to forward result to host: %v", sendErr) + return result, err + } + return result, sendErr + } + + return result, err +} + +// runJob recovers a panic in fn so ReportResult's completion guarantee holds +// even when the job crashes outright, instead of the host waiting forever +// for a result that will never arrive. +func runJob( + ctx context.Context, + fn func(ctx context.Context) (*config.Result, error), +) (result *config.Result, err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("panic: %v", r) + } + }() + return fn(ctx) +} + +func sendResult(tunnelClient tunnel.TunnelClient, result *config.Result) error { + out, err := json.Marshal(result) + if err != nil { + return err + } + + sendCtx, cancel := context.WithTimeout(context.Background(), sendResultTimeout) + defer cancel() + + message := &tunnel.Message{Message: string(out)} + if _, err := tunnelClient.SendResult(sendCtx, message); err != nil { + return fmt.Errorf("send result: %w", err) + } + return nil +} diff --git a/pkg/agent/tunnelserver/result_reporter_test.go b/pkg/agent/tunnelserver/result_reporter_test.go new file mode 100644 index 000000000..2b1de42fc --- /dev/null +++ b/pkg/agent/tunnelserver/result_reporter_test.go @@ -0,0 +1,181 @@ +package tunnelserver + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/devsy-org/devsy/pkg/agent/tunnel" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" +) + +type fakeSendResultClient struct { + tunnel.TunnelClient + sent *tunnel.Message + ctxErrAtCall error + ctxHadDeadline bool +} + +func (f *fakeSendResultClient) SendResult( + ctx context.Context, + in *tunnel.Message, + _ ...grpc.CallOption, +) (*tunnel.Empty, error) { + f.sent = in + f.ctxErrAtCall = ctx.Err() + _, f.ctxHadDeadline = ctx.Deadline() + return &tunnel.Empty{}, nil +} + +func decodeSentResult(t *testing.T, client *fakeSendResultClient) *config.Result { + t.Helper() + require.NotNil(t, client.sent) + result := &config.Result{} + require.NoError(t, json.Unmarshal([]byte(client.sent.Message), result)) + return result +} + +func TestReportResult_NilResultSuccess(t *testing.T) { + client := &fakeSendResultClient{} + + result, err := ReportResult( + context.Background(), + client, + func(context.Context) (*config.Result, error) { return nil, nil }, + ) + + require.NoError(t, err) + require.Nil(t, result) + require.Empty(t, decodeSentResult(t, client).Error) +} + +func TestReportResult_NilResultFailure_SynthesizesError(t *testing.T) { + client := &fakeSendResultClient{} + jobErr := errors.New("build: exit status 1") + + result, err := ReportResult( + context.Background(), + client, + func(context.Context) (*config.Result, error) { return nil, jobErr }, + ) + + require.ErrorIs(t, err, jobErr) + require.Nil(t, result) + require.Equal(t, jobErr.Error(), decodeSentResult(t, client).Error) +} + +func TestReportResult_RichResultFailure_PreservesFields(t *testing.T) { + client := &fakeSendResultClient{} + jobErr := errors.New("devcontainer up failed") + richResult := &config.Result{Error: jobErr.Error(), RecoveryAvailable: true} + + result, err := ReportResult( + context.Background(), + client, + func(context.Context) (*config.Result, error) { return richResult, jobErr }, + ) + + require.ErrorIs(t, err, jobErr) + require.Same(t, richResult, result) + sent := decodeSentResult(t, client) + require.Equal(t, jobErr.Error(), sent.Error) + require.True(t, sent.RecoveryAvailable) +} + +func TestReportResult_UsesContextIndependentOfCaller(t *testing.T) { + client := &fakeSendResultClient{} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := ReportResult( + ctx, + client, + func(context.Context) (*config.Result, error) { return nil, nil }, + ) + + require.NoError(t, err) + require.NoError(t, client.ctxErrAtCall) + require.True( + t, + client.ctxHadDeadline, + "expected SendResult to be called with a bounded context", + ) +} + +func TestReportResult_SendFailure_SuccessfulJobReportsSendErr(t *testing.T) { + client := &erroringSendResultClient{err: errors.New("transport down")} + + _, err := ReportResult( + context.Background(), + client, + func(context.Context) (*config.Result, error) { return nil, nil }, + ) + + require.ErrorIs(t, err, client.err) +} + +func TestReportResult_SendFailure_JobErrorTakesPrecedence(t *testing.T) { + client := &erroringSendResultClient{err: errors.New("transport down")} + jobErr := errors.New("build: exit status 1") + + _, err := ReportResult( + context.Background(), + client, + func(context.Context) (*config.Result, error) { return nil, jobErr }, + ) + + require.ErrorIs(t, err, jobErr) +} + +func TestReportResult_PanicInFnIsReported(t *testing.T) { + client := &fakeSendResultClient{} + + result, err := ReportResult( + context.Background(), + client, + func(context.Context) (*config.Result, error) { + panic("boom") + }, + ) + + require.Nil(t, result) + require.ErrorContains(t, err, "boom") + require.Contains(t, decodeSentResult(t, client).Error, "boom") +} + +func TestReportResult_DoesNotMutateCallersResult(t *testing.T) { + client := &fakeSendResultClient{} + jobErr := errors.New("devcontainer up failed") + callerResult := &config.Result{} + + _, err := ReportResult( + context.Background(), + client, + func(context.Context) (*config.Result, error) { return callerResult, jobErr }, + ) + + require.ErrorIs(t, err, jobErr) + require.Empty( + t, + callerResult.Error, + "ReportResult must not mutate the caller-owned result struct", + ) + require.Equal(t, jobErr.Error(), decodeSentResult(t, client).Error) +} + +type erroringSendResultClient struct { + tunnel.TunnelClient + err error +} + +func (f *erroringSendResultClient) SendResult( + context.Context, + *tunnel.Message, + ...grpc.CallOption, +) (*tunnel.Empty, error) { + return nil, f.err +} diff --git a/pkg/agent/tunnelserver/tunnelserver.go b/pkg/agent/tunnelserver/tunnelserver.go index 11d1de67a..9c29d15d4 100644 --- a/pkg/agent/tunnelserver/tunnelserver.go +++ b/pkg/agent/tunnelserver/tunnelserver.go @@ -14,6 +14,7 @@ import ( "path/filepath" "slices" "strings" + "sync" "github.com/devsy-org/api/pkg/devsy" "github.com/devsy-org/devsy/pkg/agent/tunnel" @@ -114,6 +115,7 @@ type tunnelServer struct { allowDockerCredentials bool allowKubeConfig bool allowPlatformOptions bool + resultMu sync.Mutex result *config.Result workspace *provider2.Workspace @@ -147,17 +149,23 @@ func (t *tunnelServer) RunWithResult( select { case err := <-errChan: - if t.result != nil { - return t.result, nil + if result := t.getResult(); result != nil { + return result, nil } return nil, err case <-ctx.Done(): - return t.result, nil + if result := t.getResult(); result != nil { + return result, nil + } + return nil, ctx.Err() } } func (t *tunnelServer) Run(ctx context.Context, reader io.Reader, writer io.WriteCloser) error { _, err := t.RunWithResult(ctx, reader, writer) + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil + } return err } @@ -409,7 +417,7 @@ func (t *tunnelServer) SendResult( return nil, err } - t.result = parsedResult + t.setResult(parsedResult) return &tunnel.Empty{}, nil } @@ -662,3 +670,15 @@ func (t *tunnelServer) workspaceIgnoreExcludes() []string { } return excludes } + +func (t *tunnelServer) getResult() *config.Result { + t.resultMu.Lock() + defer t.resultMu.Unlock() + return t.result +} + +func (t *tunnelServer) setResult(result *config.Result) { + t.resultMu.Lock() + defer t.resultMu.Unlock() + t.result = result +} diff --git a/pkg/agent/tunnelserver/tunnelserver_test.go b/pkg/agent/tunnelserver/tunnelserver_test.go index d0676b405..12fd047db 100644 --- a/pkg/agent/tunnelserver/tunnelserver_test.go +++ b/pkg/agent/tunnelserver/tunnelserver_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/devsy-org/devsy/pkg/agent/tunnel" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -72,3 +73,79 @@ func TestStreamSnapshotVolumes_TarsMountTargets(t *testing.T) { } require.True(t, found, "expected tar entry %q not found", wantName) } + +func TestRunWithResult_CancelBeforeResult(t *testing.T) { + srv := New() + + reader, writer := io.Pipe() + defer func() { _ = reader.Close() }() + defer func() { _ = writer.Close() }() + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(20 * time.Millisecond) + cancel() + }() + + result, err := srv.RunWithResult(ctx, reader, writer) + require.Nil(t, result) + require.ErrorIs(t, err, context.Canceled) +} + +func TestRun_CancelBeforeResultIsExpected(t *testing.T) { + srv := New() + + reader, writer := io.Pipe() + defer func() { _ = reader.Close() }() + defer func() { _ = writer.Close() }() + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(20 * time.Millisecond) + cancel() + }() + + require.NoError(t, srv.Run(ctx, reader, writer)) +} + +func TestRunWithResult_CancelAfterResult(t *testing.T) { + srv := New() + want := &config.Result{} + srv.setResult(want) + + reader, writer := io.Pipe() + defer func() { _ = reader.Close() }() + defer func() { _ = writer.Close() }() + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(20 * time.Millisecond) + cancel() + }() + + result, err := srv.RunWithResult(ctx, reader, writer) + require.NoError(t, err) + require.Same(t, want, result) +} + +func TestRunWithResult_ConcurrentSendResult(t *testing.T) { + srv := New() + + reader, writer := io.Pipe() + defer func() { _ = reader.Close() }() + defer func() { _ = writer.Close() }() + + ctx, cancel := context.WithCancel(context.Background()) + sendDone := make(chan error, 1) + go func() { + _, err := srv.SendResult(context.Background(), &tunnel.Message{Message: "{}"}) + sendDone <- err + cancel() + }() + + result, err := srv.RunWithResult(ctx, reader, writer) + + require.NoError(t, <-sendDone) + require.NoError(t, err) + require.NotNil(t, result) +} diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index b8bec9d2e..b8f112802 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -10,6 +10,7 @@ import ( "os" "os/exec" "strings" + "sync/atomic" "time" "github.com/devsy-org/devsy/pkg/command" @@ -238,6 +239,20 @@ type PullOptions struct { Stderr io.Writer } +func runCmd(ctx context.Context, cmd *exec.Cmd) error { + var cancelledByCtx atomic.Bool + cmd.Cancel = func() error { + cancelledByCtx.Store(true) + return cmd.Process.Kill() + } + + err := cmd.Run() + if err != nil && cancelledByCtx.Load() { + return fmt.Errorf("%w: %w", ctx.Err(), err) + } + return err +} + func (r *DockerHelper) Pull(ctx context.Context, opts PullOptions) error { args := []string{"pull"} if opts.Platform != "" { @@ -248,7 +263,7 @@ func (r *DockerHelper) Pull(ctx context.Context, opts PullOptions) error { cmd.Stdin = opts.Stdin cmd.Stdout = opts.Stdout cmd.Stderr = opts.Stderr - return cmd.Run() + return runCmd(ctx, cmd) } func (r *DockerHelper) Remove(ctx context.Context, id string) error { @@ -281,7 +296,7 @@ func (r *DockerHelper) RunWithDir( cmd.Stdin = streams.Stdin cmd.Stdout = streams.Stdout cmd.Stderr = streams.Stderr - return cmd.Run() + return runCmd(ctx, cmd) } // RunWithEnv runs a command with extra environment variables for this @@ -302,7 +317,7 @@ func (r *DockerHelper) RunWithEnv( cmd.Stdin = streams.Stdin cmd.Stdout = streams.Stdout cmd.Stderr = streams.Stderr - return cmd.Run() + return runCmd(ctx, cmd) } func (r *DockerHelper) StartContainer(ctx context.Context, containerId string) error { @@ -536,7 +551,7 @@ func (r *DockerHelper) GetContainerLogs( cmd.Stdout = stdout cmd.Stderr = stderr - return cmd.Run() + return runCmd(ctx, cmd) } // containerStateError returns an error describing the container's state, including its diff --git a/pkg/docker/helper_test.go b/pkg/docker/helper_test.go index c7db629c2..216636b1b 100644 --- a/pkg/docker/helper_test.go +++ b/pkg/docker/helper_test.go @@ -14,6 +14,10 @@ import ( "github.com/stretchr/testify/require" ) +const testFakeCommand = "cmd" + +var testExecArgs = []string{"exec", "c1", testFakeCommand} + func writeScript(t *testing.T, dir, name, script string) string { t.Helper() path := filepath.Join(dir, name) @@ -123,9 +127,6 @@ echo "$@" > `+argsFile+` func TestFindContainerJSON_MatchesAllLabels(t *testing.T) { tmp := t.TempDir() - // Fake docker: `ps -q -a` lists three containers; `inspect` returns each - // container's labels. c1 matches both query labels; c2 matches only the - // last label (an earlier label differs); c3 inspect returns an empty array. bin := writeScript(t, tmp, "docker-fake", `#!/bin/sh case "$1" in ps) printf 'c1\nc2\nc3\n' ;; @@ -142,9 +143,6 @@ esac got, err := h.FindContainerJSON(context.Background(), []string{"a=x", "b=y"}) require.NoError(t, err) - // Only c1 satisfies every label. c2 must be excluded (the AND-logic bug - // previously matched it on the last label alone), and c3's empty inspect - // result must not panic. assert.Equal(t, []string{"c1"}, got) } @@ -279,3 +277,77 @@ exit 1 assert.NoError(t, err, "should not propagate error on command failure") assert.False(t, got, "should fall back to no GPU on command failure") } + +func TestRunCmd_AttachesCtxErrOnFailure(t *testing.T) { + tmp := t.TempDir() + ready := filepath.Join(tmp, "ready") + bin := writeScript(t, tmp, "docker-fake", `#!/bin/sh +touch `+ready+` +exec sleep 30 +`) + h := &DockerHelper{DockerCommand: bin} + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + deadline := time.Now().Add(time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(ready); err == nil { + break + } + time.Sleep(time.Millisecond) + } + cancel() + }() + + streams := Streams{Stdout: io.Discard, Stderr: io.Discard} + err := h.Run(ctx, testExecArgs, streams) + + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) +} + +func TestRunCmd_AlreadyCancelledBeforeStart(t *testing.T) { + bin := writeScript(t, t.TempDir(), "docker-fake", `#!/bin/sh +exit 1 +`) + h := &DockerHelper{DockerCommand: bin} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + streams := Streams{Stdout: io.Discard, Stderr: io.Discard} + err := h.Run(ctx, testExecArgs, streams) + + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) +} + +func TestRunCmd_NoCtxErrWhenNotCancelled(t *testing.T) { + bin := writeScript(t, t.TempDir(), "docker-fake", `#!/bin/sh +exit 1 +`) + h := &DockerHelper{DockerCommand: bin} + + streams := Streams{Stdout: io.Discard, Stderr: io.Discard} + err := h.Run(context.Background(), testExecArgs, streams) + + require.Error(t, err) + assert.NotErrorIs(t, err, context.Canceled) +} + +func TestRunCmd_CancelAfterReturnNotRetroactivelyAttributed(t *testing.T) { + bin := writeScript(t, t.TempDir(), "docker-fake", `#!/bin/sh +exit 1 +`) + h := &DockerHelper{DockerCommand: bin} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + streams := Streams{Stdout: io.Discard, Stderr: io.Discard} + err := h.Run(ctx, testExecArgs, streams) + cancel() + + require.Error(t, err) + assert.NotErrorIs(t, err, context.Canceled) +} diff --git a/pkg/driver/docker/lifecycle.go b/pkg/driver/docker/lifecycle.go index aad0929b8..deafa4985 100644 --- a/pkg/driver/docker/lifecycle.go +++ b/pkg/driver/docker/lifecycle.go @@ -43,11 +43,15 @@ func (d *dockerDriver) CommandDevContainer( args = append(args, "-i") } args = append(args, "-u", params.User, container.ID, "sh", "-c", params.Command) - return d.Docker.Run(ctx, args, docker.Streams{ + err = d.Docker.Run(ctx, args, docker.Streams{ Stdin: params.Stdin, Stdout: params.Stdout, Stderr: params.Stderr, }) + if err != nil { + return fmt.Errorf("run command in container: %w", err) + } + return nil } // ensureContainerRunning checks that the given container is running, and if diff --git a/pkg/driver/docker/lifecycle_test.go b/pkg/driver/docker/lifecycle_test.go index 84b05c1ac..53638a9ef 100644 --- a/pkg/driver/docker/lifecycle_test.go +++ b/pkg/driver/docker/lifecycle_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strconv" "testing" + "time" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/docker" @@ -188,3 +189,43 @@ esac err := d.CommitContainer(context.Background(), "missing-ws", "tag:latest") require.Error(t, err) } + +func TestCommandDevContainer_CancelWrapsCtxErr(t *testing.T) { + dir := t.TempDir() + ready := filepath.Join(dir, "ready") + script := `#!/bin/sh +case "$1" in + inspect) + echo '[{"ID":"c1","State":{"Status":"running"}}]' + ;; + exec) + touch "` + ready + `" + sleep 5 + ;; +esac +` + bin := filepath.Join(dir, "docker-fake") + require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) //nolint:gosec + + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: bin, ContainerID: "c1"}} + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + deadline := time.Now().Add(time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(ready); err == nil { + break + } + time.Sleep(time.Millisecond) + } + cancel() + }() + + err := d.CommandDevContainer(ctx, &driver.CommandParams{ + User: rootUser, + Command: "true", + }) + + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) +}