diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 326bb1f309..cd8bcb90fc 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -169,6 +169,8 @@ jobs: matrix: ${{ fromJSON(needs.plan.outputs.unit) }} runs-on: ${{ matrix.runner }} timeout-minutes: ${{ matrix.timeout_minutes }} + permissions: + contents: read steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 diff --git a/cmd/provider/list_test.go b/cmd/provider/list_test.go index 8f83fe380c..7a5b972edf 100644 --- a/cmd/provider/list_test.go +++ b/cmd/provider/list_test.go @@ -5,6 +5,7 @@ import ( "errors" "io" "os" + "strings" "testing" "github.com/devsy-org/devsy/pkg/config" @@ -90,24 +91,36 @@ type providerListOutput struct { err error } -func captureProviderListOutput(t *testing.T, render func() error) providerListOutput { +func captureProviderListOutput(t *testing.T, render func() error) (output providerListOutput) { t.Helper() - read, write, err := os.Pipe() + capture, err := os.CreateTemp(t.TempDir(), "stdout") require.NoError(t, err) previous := os.Stdout - os.Stdout = write + os.Stdout = capture defer func() { os.Stdout = previous - _ = write.Close() - _ = read.Close() + output.err = errors.Join(output.err, capture.Close()) }() renderErr := render() - os.Stdout = previous - closeErr := write.Close() - data, readErr := io.ReadAll(read) - readCloseErr := read.Close() + _, seekErr := capture.Seek(0, io.SeekStart) + data, readErr := io.ReadAll(capture) return providerListOutput{ text: string(data), - err: errors.Join(renderErr, closeErr, readErr, readCloseErr), + err: errors.Join(renderErr, seekErr, readErr), } } + +func TestCaptureProviderListOutputLargeOutput(t *testing.T) { + want := strings.Repeat("0123456789abcdef", 1<<16) + renderErr := errors.New("render failed") + previous := os.Stdout + output := captureProviderListOutput(t, func() error { + n, err := io.WriteString(os.Stdout, want) + require.NoError(t, err) + require.Equal(t, len(want), n) + return renderErr + }) + require.Equal(t, want, output.text) + require.ErrorIs(t, output.err, renderErr) + require.Same(t, previous, os.Stdout) +} diff --git a/cmd/secrets/list_test.go b/cmd/secrets/list_test.go index a91fd8ce7f..bfb9d3f1c4 100644 --- a/cmd/secrets/list_test.go +++ b/cmd/secrets/list_test.go @@ -4,11 +4,13 @@ import ( "encoding/json" "io" "os" + "strings" "testing" "time" "github.com/devsy-org/devsy/pkg/config" devsysecrets "github.com/devsy-org/devsy/pkg/secrets" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -141,23 +143,37 @@ func (s *ListTestSuite) TestListEntriesReportsAvailabilityInJSONAndPlainOutput() func captureListOutput(t *testing.T, render func()) string { t.Helper() - read, write, err := os.Pipe() + capture, err := os.CreateTemp(t.TempDir(), "stdout") if err != nil { t.Fatal(err) } previous := os.Stdout - os.Stdout = write - defer func() { os.Stdout = previous }() + os.Stdout = capture + defer func() { + os.Stdout = previous + if err := capture.Close(); err != nil { + t.Error(err) + } + }() render() - if err := write.Close(); err != nil { + if _, err := capture.Seek(0, io.SeekStart); err != nil { t.Fatal(err) } - output, err := io.ReadAll(read) + output, err := io.ReadAll(capture) if err != nil { t.Fatal(err) } - if err := read.Close(); err != nil { - t.Fatal(err) - } return string(output) } + +func TestCaptureListOutputLargeOutput(t *testing.T) { + want := strings.Repeat("0123456789abcdef", 1<<16) + previous := os.Stdout + output := captureListOutput(t, func() { + n, err := io.WriteString(os.Stdout, want) + require.NoError(t, err) + require.Equal(t, len(want), n) + }) + require.Equal(t, want, output) + require.Same(t, previous, os.Stdout) +} diff --git a/cmd/workspace/helpers_test.go b/cmd/workspace/helpers_test.go index cb4f5a982a..bbbe674119 100644 --- a/cmd/workspace/helpers_test.go +++ b/cmd/workspace/helpers_test.go @@ -4,6 +4,7 @@ import ( "context" "io" "os" + "strings" "testing" "github.com/devsy-org/devsy/pkg/client" @@ -57,20 +58,36 @@ func (f *fakeWorkspaceClient) WorkspaceConfig() *provider.Workspace { return f.c func (f *fakeWorkspaceClient) Lock(context.Context) error { return nil } func (f *fakeWorkspaceClient) Unlock() {} -// captureStdout runs fn while redirecting os.Stdout to a pipe and returns -// everything written. +// captureStdout runs fn while redirecting os.Stdout to a temporary file and +// returns everything written. func captureStdout(t *testing.T, fn func()) string { t.Helper() - orig := os.Stdout - r, w, err := os.Pipe() + capture, err := os.CreateTemp(t.TempDir(), "stdout") require.NoError(t, err) - os.Stdout = w - defer func() { os.Stdout = orig }() + orig := os.Stdout + os.Stdout = capture + defer func() { + os.Stdout = orig + require.NoError(t, capture.Close()) + }() fn() - require.NoError(t, w.Close()) - out, err := io.ReadAll(r) + _, err = capture.Seek(0, io.SeekStart) + require.NoError(t, err) + out, err := io.ReadAll(capture) require.NoError(t, err) return string(out) } + +func TestCaptureStdoutLargeOutput(t *testing.T) { + want := strings.Repeat("0123456789abcdef", 1<<16) + orig := os.Stdout + got := captureStdout(t, func() { + n, err := io.WriteString(os.Stdout, want) + require.NoError(t, err) + require.Equal(t, len(want), n) + }) + require.Equal(t, want, got) + require.Same(t, orig, os.Stdout) +} diff --git a/cmd/workspace/task.go b/cmd/workspace/task.go index 39105ba3d5..0fed237d99 100644 --- a/cmd/workspace/task.go +++ b/cmd/workspace/task.go @@ -12,6 +12,7 @@ import ( 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" "github.com/devsy-org/devsy/pkg/output" "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/table" @@ -181,8 +182,14 @@ type followTaskOptions struct { emitJSON bool } -func followTask(ctx context.Context, store *task.Store, opts followTaskOptions) error { +type taskFollowerStore interface { + ReconcileTask(context.Context, string) (*task.State, error) + Get(string) (*task.State, error) +} + +func followTask(ctx context.Context, store taskFollowerStore, opts followTaskOptions) error { var last *task.State + var lastReconcileError error ticker := time.NewTicker(opts.interval) defer ticker.Stop() @@ -195,7 +202,7 @@ func followTask(ctx context.Context, store *task.Store, opts followTaskOptions) for { tailer.poll(os.Stderr) - state, err := store.Get(opts.id) + state, err := readTaskForFollower(ctx, store, opts.id, &lastReconcileError) if err != nil { return err } @@ -216,6 +223,29 @@ func followTask(ctx context.Context, store *task.Store, opts followTaskOptions) } } +func readTaskForFollower( + ctx context.Context, + store taskFollowerStore, + id string, + lastReconcileError *error, +) (*task.State, error) { + state, err := store.ReconcileTask(ctx, id) + if err == nil { + *lastReconcileError = nil + return state, nil + } + if ctx.Err() != nil { + return nil, ctx.Err() + } + message := err.Error() + if *lastReconcileError == nil || message != (*lastReconcileError).Error() { + log.Debugf("Reconcile task %s: %s", id, message) + *lastReconcileError = err + } + // Cleanup can be retried; only persisted terminal state ends the follower. + return store.Get(id) +} + func emitTaskTransition(last, current *task.State, emitJSON bool) { if last != nil && last.Phase == current.Phase && last.Step == current.Step && last.Status == current.Status && last.OperationID == current.OperationID && diff --git a/cmd/workspace/task_tail_test.go b/cmd/workspace/task_tail_test.go index 429b5c436f..f87c355652 100644 --- a/cmd/workspace/task_tail_test.go +++ b/cmd/workspace/task_tail_test.go @@ -3,6 +3,8 @@ package workspace import ( "bytes" "context" + "encoding/json" + "errors" "io" "os" "path/filepath" @@ -10,8 +12,11 @@ import ( "testing" "time" + "github.com/devsy-org/devsy/pkg/clierr" "github.com/devsy-org/devsy/pkg/config" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/secrets" "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/task" "github.com/stretchr/testify/require" @@ -24,6 +29,7 @@ func newTestTailer(t *testing.T, taskID string) (*logTailer, string) { t.Cleanup(config.ResetPathManager) tailer := newLogTailer(taskID) + t.Cleanup(func() { tailer.flush(io.Discard) }) if tailer.path == "" { t.Fatal("newLogTailer produced an empty path") } @@ -290,3 +296,245 @@ func TestFollowTaskDrainsStatusBeforeResult(t *testing.T) { require.Equal(t, "ide", completion.OperationID) require.Contains(t, lines[2], `"kind":"result"`) } + +func TestFollowTaskReconcilesExitedWorker(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + require.NoError(t, tk.ReleaseWorkerLock()) + streamsPath, err := config.DefaultPathManager(). + ProcessStreamsFile(task.WorkerProcessName(tk.ID())) + require.NoError(t, err) + require.NoError(t, os.WriteFile(streamsPath, []byte("startup failed\n"), 0o600)) + r, w, err := os.Pipe() + require.NoError(t, err) + original := os.Stderr + os.Stderr = w + t.Cleanup(func() { os.Stderr = original; _ = r.Close(); _ = w.Close() }) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + followErr := followTask( + ctx, + store, + followTaskOptions{id: tk.ID(), interval: time.Millisecond, emitJSON: true}, + ) + os.Stderr = original + require.NoError(t, w.Close()) + logs, err := io.ReadAll(r) + require.NoError(t, err) + require.Error(t, followErr) + require.NotErrorIs(t, followErr, context.DeadlineExceeded) + require.Equal(t, 1, strings.Count(string(logs), "startup failed")) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusFailed, state.Status) +} + +type failingReconciliationStore struct { + *task.Store + reconcile func(context.Context, string) (*task.State, error) +} + +func (s failingReconciliationStore) ReconcileTask( + ctx context.Context, + id string, +) (*task.State, error) { + return s.reconcile(ctx, id) +} + +func TestFollowTaskRetriesCleanupBeforeTerminalFailure(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + require.NoError(t, tk.ReleaseWorkerLock()) + path, err := config.DefaultPathManager().ProcessStreamsFile(task.WorkerProcessName(tk.ID())) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, []byte("captured startup failure\n"), 0o600)) + transient := errors.New("transient cleanup failure") + calls := 0 + wrapper := failingReconciliationStore{ + Store: store, + reconcile: func(ctx context.Context, id string) (*task.State, error) { + calls++ + if calls == 1 { + return nil, transient + } + state, err := store.Get(id) + require.NoError(t, err) + require.False(t, state.Status.Terminal(), "cleanup failure must leave task retryable") + return store.ReconcileTask(ctx, id) + }, + } + reader, writer, err := os.Pipe() + require.NoError(t, err) + original := os.Stderr + os.Stderr = writer + t.Cleanup(func() { os.Stderr = original; _ = reader.Close(); _ = writer.Close() }) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + followErr := followTask( + ctx, + wrapper, + followTaskOptions{id: tk.ID(), interval: time.Millisecond, emitJSON: true}, + ) + os.Stderr = original + require.NoError(t, writer.Close()) + logs, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, 1, strings.Count(string(logs), "captured startup failure")) + require.Error(t, followErr) + require.NotErrorIs(t, followErr, transient) + require.NotErrorIs(t, followErr, context.DeadlineExceeded) + var classified *clierr.CLIError + require.ErrorAs(t, followErr, &classified) + require.Equal(t, 2, calls) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusFailed, state.Status) + require.Equal(t, state.Error, classified.Message) +} + +func TestFollowTaskRetryHonorsCancellation(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{}) + require.NoError(t, err) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + calls := 0 + wrapper := failingReconciliationStore{ + Store: store, + reconcile: func(context.Context, string) (*task.State, error) { + calls++ + if calls == 2 { + cancel() + } + return nil, errors.New("cleanup unavailable") + }, + } + err = followTask(ctx, wrapper, followTaskOptions{id: tk.ID(), interval: time.Millisecond}) + require.ErrorIs(t, err, context.Canceled) + require.Equal(t, 2, calls) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.False(t, state.Status.Terminal()) +} + +func TestFollowTaskReconciliationDiagnostics(t *testing.T) { + for _, level := range []string{"debug", "info"} { + t.Run(level, func(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + secret := strings.Repeat("test-value-", 8) + reader, writer, err := os.Pipe() + require.NoError(t, err) + original := os.Stdout + os.Stdout = writer + t.Cleanup(func() { os.Stdout = original; _ = reader.Close(); _ = writer.Close() }) + log.InitTest(t) + log.Init( + log.Config{ + Level: level, + Format: "json", + Redactor: secrets.NewRedactor([]string{"TOKEN=" + secret}), + }, + ) + var diagnostics bytes.Buffer + remove := log.AddSink(&diagnostics) + defer remove() + // Each follower must report its own first failure, suppress consecutive + // duplicates, and report a recurrence after successful reconciliation. + for range 2 { + runFollowerReconciliationSequence(t, secret) + } + _ = log.Sync() + os.Stdout = original + require.NoError(t, writer.Close()) + output, err := io.ReadAll(reader) + require.NoError(t, err) + lines := strings.Split(strings.TrimSpace(string(output)), "\n") + require.Len(t, lines, 2, "only terminal result envelopes belong on stdout") + for _, line := range lines { + var envelope map[string]any + require.NoError(t, json.Unmarshal([]byte(line), &envelope)) + require.Equal(t, "result", envelope["kind"]) + require.NotContains(t, line, "cleanup") + } + requireReconciliationDiagnostics(t, level, diagnostics.String(), secret) + }) + } +} + +func requireReconciliationDiagnostics(t *testing.T, level, diagnostics, secret string) { + t.Helper() + require.NotContains(t, diagnostics, secret) + if level == "info" { + require.Empty(t, diagnostics) + return + } + records := strings.Split(strings.TrimSpace(diagnostics), "\n") + require.Len(t, records, 6) + for i, line := range records { + var record map[string]any + require.NoError(t, json.Unmarshal([]byte(line), &record)) + require.Equal(t, "debug", record["level"]) + require.NotContains(t, record, "kind") + if i%3 == 1 { + require.Contains(t, record["msg"], "lock unavailable") + } else { + require.Contains(t, record["msg"], "cleanup ***") + } + } +} + +func runFollowerReconciliationSequence(t *testing.T, secret string) { + t.Helper() + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{}) + require.NoError(t, err) + sequence := []string{ + "cleanup " + secret, + "cleanup " + secret, + "lock unavailable", + "lock unavailable", + "", + "cleanup " + secret, + "cleanup " + secret, + } + calls := 0 + wrapper := failingReconciliationStore{ + Store: store, + reconcile: func(_ context.Context, id string) (*task.State, error) { + if calls == len(sequence) { + require.NoError(t, tk.Succeed(nil)) + return store.Get(id) + } + message := sequence[calls] + calls++ + if message == "" { + return store.Get(id) + } + return nil, errors.New(message) + }, + } + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + followErr := followTask( + ctx, + wrapper, + followTaskOptions{id: tk.ID(), interval: time.Millisecond, emitJSON: true}, + ) + cancel() + require.NoError(t, followErr) + require.Equal(t, len(sequence), calls) +} diff --git a/cmd/workspace/up/detach.go b/cmd/workspace/up/detach.go index 84b7eddffd..e12c0a052e 100644 --- a/cmd/workspace/up/detach.go +++ b/cmd/workspace/up/detach.go @@ -1,6 +1,7 @@ package up import ( + "context" "errors" "fmt" "os" @@ -36,7 +37,7 @@ func (cmd *UpCmd) runDetached(args []string, cfg *config.Config) error { if err != nil { return err } - t, err := store.Create(task.CreateOptions{ + t, err := store.CreateLaunch(task.CreateOptions{ Command: "up", WorkspaceID: workspaceID, }) @@ -44,14 +45,8 @@ func (cmd *UpCmd) runDetached(args []string, cfg *config.Config) error { return err } - if err := t.BeginLaunch(); err != nil { - _ = t.Fail(err) - return fmt.Errorf("prepare detached up: %w", err) - } if err := launchDetached(t, cmd.detachedInvocationEnvironment()); err != nil { - _ = t.FinishLaunch() - _ = t.Fail(err) - return fmt.Errorf("launch detached up: %w", err) + return errors.Join(fmt.Errorf("launch detached up: %w", err), t.FailLaunch(err)) } return cmd.renderDetachedTask(t) } @@ -74,6 +69,9 @@ func (cmd *UpCmd) prepareDetached(cfg *config.Config) error { if err := mergeDevsyUpOptions(&cmd.CLIOptions); err != nil { return err } + if err := workspace2.ValidateID(cmd.ID); err != nil { + return err + } return cmd.preflightLocalValues(cfg) } @@ -85,9 +83,17 @@ func launchDetached(t *task.Task, invocationEnv []string) error { args := append(detachedArgs(os.Args[1:]), names.Flag(names.TaskID), t.ID()) return command.StartSupervisedBackground(command.SupervisedStartOptions{ - Name: task.WorkerProcessName(t.ID()), + Name: task.WorkerProcessName(t.ID()), + Lease: t.LaunchLease(), + AtomicOwnership: true, + OnPrepared: t.PrepareProcessOwnership, OnStarted: func(ref command.ProcessRef) error { - return t.SetProcess(ref) + if err := t.PublishProcess(ref); err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return t.CommitLaunch(ctx) }, }, func() (*exec.Cmd, error) { return &exec.Cmd{ @@ -149,58 +155,152 @@ func (cmd *UpCmd) openTask() (*task.Task, error) { } t := store.Open(cmd.taskID) if err := t.HoldWorkerLock(); err != nil { - failTask(t, err) return nil, err } + verifiedOwner, err := waitForTaskLaunch(t, store) + if err != nil { + if verifiedOwner { + return nil, finalizeWorkerTask(t, err) + } + if releaseErr := t.ReleaseWorkerLock(); releaseErr != nil { + err = errors.Join(err, fmt.Errorf("release detached worker lock: %w", releaseErr)) + } + return nil, err + } + return t, nil +} + +type workerLaunchProgress struct { + leaseRetained bool + verifiedOwner bool +} + +func waitForTaskLaunch(t *task.Task, store *task.Store) (bool, error) { deadline := time.NewTimer(5 * time.Second) defer deadline.Stop() ticker := time.NewTicker(20 * time.Millisecond) defer ticker.Stop() + progress := workerLaunchProgress{} for { - state, err := store.Get(cmd.taskID) + state, err := readWorkerLaunchState(t, store) if err != nil { - failTask(t, err) - _ = t.ReleaseWorkerLock() - return nil, err + return progress.verifiedOwner, err } - if state.Status.Terminal() || state.CancelRequested { - if err := t.ReleaseWorkerLock(); err != nil { - return nil, fmt.Errorf("release canceled worker lock: %w", err) - } - return nil, task.ErrCanceled + ready, err := advanceTaskLaunch(t, store, state, &progress) + if err != nil { + return progress.verifiedOwner, err } - if !state.LaunchPending { - if _, ok := state.ProcessReference(); !ok { - err := errors.New("detached worker launch did not publish process metadata") - failTask(t, err) - _ = t.ReleaseWorkerLock() - return nil, err - } - return t, nil + if ready { + return progress.verifiedOwner, nil } select { case <-deadline.C: - err := errors.New("timed out waiting for detached worker launch metadata") - failTask(t, err) - _ = t.ReleaseWorkerLock() - return nil, err + return progress.verifiedOwner, errors.New( + "timed out waiting for detached worker launch metadata", + ) case <-ticker.C: } } } -func failTask(t *task.Task, err error) { - if t == nil { - return +func readWorkerLaunchState(t *task.Task, store *task.Store) (*task.State, error) { + state, err := store.Get(t.ID()) + if err != nil { + return nil, err + } + if state.Status.Terminal() || state.CancelRequested { + return nil, task.ErrCanceled + } + return state, nil +} + +func advanceTaskLaunch( + t *task.Task, + store *task.Store, + state *task.State, + progress *workerLaunchProgress, +) (bool, error) { + owner, err := verifyWorkerTaskOwner(state) + progress.verifiedOwner = owner + if err != nil { + return false, err + } + if !state.LaunchPending { + return true, nil + } + if state.LaunchProtocol != 1 { + progress.verifiedOwner = false + return false, nil + } + if err := retainWorkerLaunchLease(&progress.leaseRetained); err != nil { + return false, err + } + if err := checkWorkerLauncher(t, store); err != nil { + return false, err + } + if state.PID != 0 && !state.WorkerReady { + return false, t.WorkerLaunchReady(os.Getpid()) + } + return false, nil +} + +// The worker lock excludes concurrent invocations; only the saved kernel identity +// authorizes this invocation to acknowledge launch or persist startup failures. +func verifyWorkerTaskOwner(state *task.State) (bool, error) { + ref, ok := state.ProcessReference() + if !ok { + if state.LaunchPending { + return false, nil + } + return false, errors.New("detached worker launch did not publish process metadata") + } + if ref.PID != os.Getpid() || ref.Identity == "" { + return false, errors.New("detached task belongs to a different or unverified worker") + } + identity, err := command.ProcessTreeIdentity(os.Getpid()) + if err != nil { + return false, fmt.Errorf("verify detached worker identity: %w", err) + } + if identity == "" || identity != ref.Identity { + return false, errors.New("detached worker identity does not match task ownership") + } + return true, nil +} + +func retainWorkerLaunchLease(retained *bool) error { + if *retained { + return nil + } + if err := command.RetainLaunchLease(); err != nil { + return err + } + *retained = true + return nil +} + +func checkWorkerLauncher(t *task.Task, store *task.Store) error { + alive, err := t.LauncherAlive() + if err != nil { + return err + } + if alive { + return nil + } + latest, err := store.Get(t.ID()) + if err != nil { + return err + } + if latest.LaunchPending { + return errors.New("detached worker launcher exited before commit") } - _ = t.Fail(err) + return nil } -func succeedTask(t *task.Task, result *config2.Result) { +func succeedTask(t *task.Task, result *config2.Result) error { if t == nil { - return + return nil } - _ = t.Succeed(result) + return t.Succeed(result) } func (cmd *UpCmd) detachedInvocationEnvironment() []string { diff --git a/cmd/workspace/up/early_task_test.go b/cmd/workspace/up/early_task_test.go new file mode 100644 index 0000000000..bc4b8accf7 --- /dev/null +++ b/cmd/workspace/up/early_task_test.go @@ -0,0 +1,175 @@ +package up + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/devsy-org/devsy/cmd/flags" + "github.com/devsy-org/devsy/pkg/clierr" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/task" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func isolatedTaskStore(t *testing.T) *task.Store { + t.Helper() + t.Setenv("DEVSY_HOME", t.TempDir()) + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) + store, err := task.NewStore() + require.NoError(t, err) + return store +} + +func TestDetachedRejectsInvalidIDBeforeCreatingTask(t *testing.T) { + for _, id := range []string{"Backend", "backend_name", "backend.api", "has spaces", strings.Repeat("a", 49)} { + t.Run(id, func(t *testing.T) { + store := isolatedTaskStore(t) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.ID = id + cmd.ResultFormat = "json" + err := cmd.runDetached(nil, &config.Config{}) + assert.ErrorContains(t, err, "workspace name") + tasks, err := store.List() + require.NoError(t, err) + require.Empty(t, tasks) + }) + } +} + +func TestWorkerRecordsValidationFailureBeforeRun(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}, taskID: tk.ID()} + cmd.WorkspaceMountConsistency = "invalid" + cobraCmd := &cobra.Command{} + cobraCmd.SetContext(context.Background()) + err = cmd.execute(cobraCmd, nil) + require.ErrorContains(t, err, "workspace-mount-consistency") + state, getErr := store.Get(tk.ID()) + require.NoError(t, getErr) + require.Equal(t, task.StatusFailed, state.Status) + require.Contains(t, state.Error, "workspace-mount-consistency") + assertWorkerLockReleased(t, store, tk.ID()) +} + +func TestDuplicateWorkerCannotFailOwnerTask(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + require.NoError(t, tk.HoldWorkerLock()) + defer func() { require.NoError(t, tk.ReleaseWorkerLock()) }() + cmd := &UpCmd{taskID: tk.ID()} + _, err = cmd.openTask() + require.ErrorContains(t, err, "already has a running worker") + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusPending, state.Status) +} + +func TestDetachedValidatesMergedID(t *testing.T) { + isolatedTaskStore(t) + t.Setenv(config.EnvFlagsUp, `{"id":"Backend"}`) + cmd := &UpCmd{} + cmd.ID = "backend" + require.ErrorContains(t, cmd.prepareDetached(nil), "lowercase") + require.Equal(t, "Backend", cmd.ID) +} + +func TestWorkerRecordsConfigAndClientErrors(t *testing.T) { + for _, stage := range []string{"config", "client", "provider"} { + t.Run(stage, func(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}, taskID: tk.ID()} + cmd.ResultFormat = "json" + expected := "prepare workspace client" + switch stage { + case "config": + path, pathErr := config.GetConfigPath() + require.NoError(t, pathErr) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + require.NoError(t, os.WriteFile(path, []byte("contexts: ["), 0o600)) + expected = "load devsy config" + case "client": + cmd.ID = "Backend" + case "provider": + cmd.Provider = "missing-provider" + } + cobraCmd := &cobra.Command{} + cobraCmd.SetContext(context.Background()) + err = cmd.execute(cobraCmd, []string{t.TempDir()}) + require.ErrorContains(t, err, expected) + state, getErr := store.Get(tk.ID()) + require.NoError(t, getErr) + require.Equal(t, task.StatusFailed, state.Status) + require.Contains(t, state.Error, expected) + assertWorkerLockReleased(t, store, tk.ID()) + }) + } +} + +func TestFinalizeWorkerPreservesClassifiedRedactedError(t *testing.T) { + store := isolatedTaskStore(t) + testValue := strings.Repeat("test-value-", 8) + t.Setenv("DEVSY_SECRETS_PASSPHRASE", testValue) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + original := &clierr.CLIError{ + Code: clierr.CodeCanceled, + Message: testValue, + Hint: "retry " + testValue, + Context: map[string]string{"source": testValue}, + } + require.ErrorIs(t, finalizeWorkerTask(tk, original), original) + second := errors.New("second failure") + require.ErrorIs(t, finalizeWorkerTask(tk, second), second) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, string(clierr.CodeCanceled), state.ErrorCode) + require.NotContains(t, state.Error, testValue) + require.NotContains(t, state.ErrorHint, testValue) + require.NotContains(t, state.ErrorContext["source"], testValue) + assertWorkerLockReleased(t, store, tk.ID()) +} + +func TestDetachedAllowsOmittedAndMaximumLengthID(t *testing.T) { + for _, id := range []string{"", strings.Repeat("a", 48)} { + t.Run(id, func(t *testing.T) { + isolatedTaskStore(t) + cfg, err := config.LoadConfig("", "") + require.NoError(t, err) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.ID = id + require.NoError(t, cmd.prepareDetached(cfg)) + require.Equal(t, id, cmd.ID) + }) + } +} + +func TestFinalizeWorkerSurfacesPersistenceFailure(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + dir, err := config.DefaultPathManager().TaskDir() + require.NoError(t, err) + require.NoError(t, os.Remove(filepath.Join(dir, tk.ID()+".json"))) + original := errors.New("original worker failure") + err = finalizeWorkerTask(tk, original) + require.ErrorIs(t, err, original) + require.ErrorContains(t, err, "record detached task failure") + assertWorkerLockReleased(t, store, tk.ID()) +} diff --git a/cmd/workspace/up/launch_handoff_test.go b/cmd/workspace/up/launch_handoff_test.go new file mode 100644 index 0000000000..bb1b3e455d --- /dev/null +++ b/cmd/workspace/up/launch_handoff_test.go @@ -0,0 +1,60 @@ +package up + +import ( + "context" + "os" + "os/exec" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/task" + "github.com/stretchr/testify/require" +) + +func TestDetachedWorkerCommitsBeforeWorkspaceWork(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.CreateLaunch(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + err = command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: task.WorkerProcessName(tk.ID()), Lease: tk.LaunchLease(), AtomicOwnership: true, + OnPrepared: tk.PrepareProcessOwnership, + OnStarted: func(ref command.ProcessRef) error { + if err := tk.PublishProcess(ref); err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + return tk.CommitLaunch(ctx) + }, + }, func() (*exec.Cmd, error) { + child := exec.Command( + os.Args[0], + "-test.run=^TestDetachedWorkerHandoffHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + child.Env = append(os.Environ(), "DEVSY_TEST_HANDOFF_ID="+tk.ID()) + return child, nil + }) + require.NoError(t, err) + t.Cleanup(func() { _ = tk.Cancel() }) + require.Eventually(t, func() bool { + state, err := store.Get(tk.ID()) + return err == nil && state.Status == task.StatusSucceeded + }, 10*time.Second, 20*time.Millisecond) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.WorkerReady) + require.False(t, state.LaunchPending) +} + +func TestDetachedWorkerHandoffHelper(t *testing.T) { + id := os.Getenv("DEVSY_TEST_HANDOFF_ID") + if id == "" { + return + } + cmd := &UpCmd{taskID: id} + tk, err := cmd.openTask() + require.NoError(t, err) + require.NoError(t, tk.Succeed(nil)) + require.NoError(t, tk.ReleaseWorkerLock()) +} diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 5192bba095..907d2de5cf 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -2,6 +2,7 @@ package up import ( "context" + "errors" "fmt" "io" "os" @@ -63,7 +64,8 @@ type UpCmd struct { // See cmd.runDetached. Detach bool // Set only on the detached child re-exec. - taskID string + taskID string + workerTask *task.Task // Out receives result/error JSON envelopes; nil falls back to os.Stdout. Out io.Writer // Set at the start of Run; nil until then. @@ -261,6 +263,25 @@ func (cmd *UpCmd) Run( devsyConfig *config.Config, client client2.BaseWorkspaceClient, args []string, +) (retErr error) { + if cmd.workerTask == nil && cmd.taskID != "" { + t, err := cmd.openTask() + if err != nil { + return err + } + cmd.workerTask = t + defer func() { + retErr = finalizeWorkerTask(t, retErr) + cmd.workerTask = nil + }() + } + return cmd.runWorkspace(ctx, devsyConfig, client) +} + +func (cmd *UpCmd) runWorkspace( + ctx context.Context, + devsyConfig *config.Config, + client client2.BaseWorkspaceClient, ) error { cmd.prepareWorkspace(client) @@ -276,7 +297,6 @@ func (cmd *UpCmd) Run( wctx, err := cmd.executeDevsyUp(ctx, devsyConfig, client) if err != nil { - failTask(t, err) return reportErr(err, emitJSON, out) } if wctx == nil || cmd.Prebuild { @@ -291,11 +311,9 @@ func (cmd *UpCmd) Run( out: out, }) if err != nil { - failTask(t, err) return err } - succeedTask(t, wctx.result) - return nil + return succeedTask(t, wctx.result) } func (cmd *UpCmd) configureRun(workspaceID string) (bool, io.Writer, error) { @@ -324,28 +342,22 @@ func (cmd *UpCmd) finishWithoutWorkspace( if err := status.Run(ctx, cmd.reporter(), status.Operation{ Phase: status.PhaseReady, }, func(context.Context) error { return nil }); err != nil { - failTask(t, err) return reportErr(err, emitJSON, out) } - succeedTask(t, nil) - return nil // Platform mode or prebuild-only run. + return succeedTask(t, nil) } -// setUpTask opens the run's task (if any), tees the status reporter into it, +// setUpTask tees the status reporter into the claimed task (if any), // and records the workspace ID on it. func (cmd *UpCmd) setUpTask( client client2.BaseWorkspaceClient, emitJSON bool, out io.Writer, ) (*task.Task, error) { - t, err := cmd.openTask() - if err != nil { - return nil, reportErr(err, emitJSON, out) - } + t := cmd.workerTask if t != nil { cmd.statusReporter = status.Tee(cmd.statusReporter, t.Reporter()) if err := t.SetWorkspaceID(client.Workspace()); err != nil { - failTask(t, err) return nil, reportErr(err, emitJSON, out) } } @@ -535,7 +547,21 @@ func emitUpResult(wctx *workspaceContext, ideURL string, out io.Writer) { }) } -func (cmd *UpCmd) execute(cobraCmd *cobra.Command, args []string) error { +func (cmd *UpCmd) execute(cobraCmd *cobra.Command, args []string) (retErr error) { + t, err := cmd.openTask() + if err != nil { + return err + } + cmd.workerTask = t + defer func() { + retErr = finalizeWorkerTask(t, retErr) + cmd.workerTask = nil + }() + + return cmd.executeUp(cobraCmd, args) +} + +func (cmd *UpCmd) executeUp(cobraCmd *cobra.Command, args []string) error { if err := cmd.validate(); err != nil { return err } @@ -752,3 +778,18 @@ func watchForForceShutdown(ctx context.Context, done <-chan struct{}) { signal.Stop(signals) } } + +func finalizeWorkerTask(t *task.Task, err error) error { + if t == nil { + return err + } + if err != nil { + if failErr := t.Fail(err); failErr != nil { + err = errors.Join(err, fmt.Errorf("record detached task failure: %w", failErr)) + } + } + if releaseErr := t.ReleaseWorkerLock(); releaseErr != nil { + err = errors.Join(err, fmt.Errorf("release detached worker lock: %w", releaseErr)) + } + return err +} diff --git a/cmd/workspace/up/up_client.go b/cmd/workspace/up/up_client.go index b338cce009..96c5589ab5 100644 --- a/cmd/workspace/up/up_client.go +++ b/cmd/workspace/up/up_client.go @@ -147,6 +147,9 @@ func (cmd *UpCmd) prepareClientEnvironment( if err := mergeDevsyUpOptions(&cmd.CLIOptions); err != nil { return err } + if err := workspace2.ValidateID(cmd.ID); err != nil { + return err + } if cmd.Platform.Enabled { log.Debug("running in platform mode") config.MergeContextOptions(devsyConfig.Current(), os.Environ()) diff --git a/cmd/workspace/up/worker_admission_test.go b/cmd/workspace/up/worker_admission_test.go new file mode 100644 index 0000000000..3902783306 --- /dev/null +++ b/cmd/workspace/up/worker_admission_test.go @@ -0,0 +1,199 @@ +package up + +import ( + "encoding/json" + "io" + "os" + "os/exec" + "strconv" + "testing" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/task" + "github.com/stretchr/testify/require" +) + +type admissionReferenceKind int + +const ( + admissionNoReference admissionReferenceKind = iota + admissionLiveReference + admissionExitedReference + admissionStaleReference + admissionWeakReference +) + +func TestDetachedAdmissionRejectsUnverifiedWorker(t *testing.T) { + cases := []struct { + name string + reference admissionReferenceKind + protocol int + pending bool + canceled bool + }{ + {name: "live-legacy", reference: admissionLiveReference}, + {name: "exited-legacy", reference: admissionExitedReference}, + {name: "committed-protocol", reference: admissionLiveReference, protocol: 1}, + {name: "stale-identity", reference: admissionStaleReference}, + {name: "weak-identity", reference: admissionWeakReference}, + {name: "missing-reference", reference: admissionNoReference}, + {name: "pending-mismatch", reference: admissionLiveReference, protocol: 1, pending: true}, + {name: "unpublished", reference: admissionNoReference, protocol: 1, pending: true}, + {name: "canceled-mismatch", reference: admissionLiveReference, canceled: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + state.Process = detachedAdmissionReference(t, tc.reference) + if state.Process != nil { + state.PID = state.Process.PID + state.ProcessTreeIdentity = state.Process.Identity + } + state.LaunchProtocol, state.LaunchPending = tc.protocol, tc.pending + state.CancelRequested = tc.canceled + writeDetachedAdmissionState(t, state) + admitted, err := (&UpCmd{taskID: tk.ID()}).openTask() + if admitted != nil { + require.NoError(t, admitted.ReleaseWorkerLock()) + } + require.Error(t, err) + require.Nil(t, admitted) + after, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, state, after, "unverified invocation must not mutate the saved owner") + assertWorkerLockReleased(t, store, tk.ID()) + }) + } +} + +func detachedAdmissionReference(t *testing.T, kind admissionReferenceKind) *command.ProcessRef { + t.Helper() + if kind == admissionNoReference { + return nil + } + if kind == admissionLiveReference || kind == admissionExitedReference { + ref := detachedAdmissionChild(t, kind == admissionExitedReference) + return &ref + } + identity, err := command.ProcessTreeIdentity(os.Getpid()) + require.NoError(t, err) + if kind == admissionStaleReference { + identity += "-stale" + } else { + identity = "" + } + return &command.ProcessRef{PID: os.Getpid(), Identity: identity} +} + +func TestDetachedAdmissionAcceptsCurrentIdentity(t *testing.T) { + for _, protocol := range []int{0, 1} { + t.Run(strconv.Itoa(protocol), func(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + state.LaunchProtocol = protocol + writeDetachedAdmissionState(t, state) + admitted, err := (&UpCmd{taskID: tk.ID()}).openTask() + require.NoError(t, err) + require.NotNil(t, admitted) + require.NoError(t, admitted.ReleaseWorkerLock()) + }) + } +} + +func writeDetachedAdmissionState(t *testing.T, state *task.State) { + t.Helper() + dir, err := config.DefaultPathManager().TaskDir() + require.NoError(t, err) + root, err := os.OpenRoot(dir) + require.NoError(t, err) + defer func() { require.NoError(t, root.Close()) }() + data, err := json.Marshal(state) + require.NoError(t, err) + require.NoError(t, root.WriteFile(state.ID+".json", data, 0o600)) +} + +func detachedAdmissionChild(t *testing.T, exited bool) command.ProcessRef { + t.Helper() + child := exec.Command( + os.Args[0], "-test.run=^TestDetachedAdmissionChild$", + ) // #nosec G204 G702 -- re-exec this test binary with a fixed helper selector + child.Env = append(os.Environ(), "DEVSY_TEST_ADMISSION_CHILD=1") + stdin, err := child.StdinPipe() + require.NoError(t, err) + require.NoError(t, child.Start()) + t.Cleanup(func() { + _ = stdin.Close() + _ = child.Process.Kill() + _ = child.Wait() + }) + identity, err := command.ProcessTreeIdentity(child.Process.Pid) + require.NoError(t, err) + require.NotEmpty(t, identity) + ref := command.ProcessRef{PID: child.Process.Pid, Identity: identity} + if exited { + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + gone, err := command.ProcessExited(ref) + require.NoError(t, err) + require.True(t, gone) + } + return ref +} + +func TestDetachedAdmissionChild(t *testing.T) { + if os.Getenv("DEVSY_TEST_ADMISSION_CHILD") != "1" { + return + } + _, err := io.Copy(io.Discard, os.Stdin) + require.NoError(t, err) +} + +func TestDetachedAdmissionOwnedBootstrapErrorPersists(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + state.LaunchProtocol, state.LaunchPending = 1, true + writeDetachedAdmissionState(t, state) + admitted, err := (&UpCmd{taskID: tk.ID()}).openTask() + require.Error(t, err) + require.Nil(t, admitted) + after, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusFailed, after.Status) + require.Equal(t, state.Process, after.Process) + require.True(t, after.LaunchPending) + require.False(t, after.WorkerReady) + assertWorkerLockReleased(t, store, tk.ID()) +} + +func TestDetachedAdmissionKeepsLegacyPendingUnverified(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + state.LaunchPending = true + writeDetachedAdmissionState(t, state) + progress := workerLaunchProgress{} + ready, err := advanceTaskLaunch(tk, store, state, &progress) + require.NoError(t, err) + require.False(t, ready) + require.False(t, progress.verifiedOwner) + require.False(t, progress.leaseRetained) + after, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, state, after) +} diff --git a/desktop/e2e/workspace-id.e2e.ts b/desktop/e2e/workspace-id.e2e.ts new file mode 100644 index 0000000000..0b7b9bec9d --- /dev/null +++ b/desktop/e2e/workspace-id.e2e.ts @@ -0,0 +1,82 @@ +import type { ElectronApplication } from "@playwright/test" +import { expect, test } from "@playwright/test" +import { launchApp, resetMockState } from "./electron-app.js" + +for (const [kind, source] of [ + ["git", "https://example.com/Backend.git"], + ["local", "/var/home/dev/Projects/Backend"], +]) { + test(`validates explicit IDs and preserves the ${kind} source`, async () => { + resetMockState() + let app: ElectronApplication | undefined + try { + const launched = await launchApp() + app = launched.app + const page = launched.page + await page.click('[data-sidebar="sidebar"] a[href="#/workspaces"]') + await page + .getByRole("button", { name: "Create Workspace", exact: true }) + .click() + const dialog = page.getByRole("dialog") + await dialog + .getByRole("button", { name: "Continue", exact: true }) + .click() + if (kind === "local") + await dialog.getByText("Local Directory", { exact: true }).click() + await dialog + .getByLabel( + kind === "local" ? "Local Directory *" : "Repository URL *", + { exact: true }, + ) + .fill(source) + await dialog + .getByRole("button", { name: "Continue", exact: true }) + .click() + await dialog + .getByRole("button", { name: "Continue", exact: true }) + .click() + const name = dialog.getByLabel("Workspace Name", { exact: true }) + await expect(name).toHaveValue("backend") + await expect(dialog.getByText(source, { exact: true })).toBeVisible() + await name.fill("Backend") + await expect(name).toHaveValue("Backend") + await expect(name).toHaveAttribute("aria-invalid", "true") + await expect(name).toHaveAttribute( + "aria-describedby", + "workspace-name-invalid", + ) + await expect(dialog.locator("#workspace-name-invalid")).toContainText( + 'Try "backend"', + ) + const launch = dialog.getByRole("button", { name: "Launch", exact: true }) + await expect(launch).toBeDisabled() + await name.fill("backend") + await expect(launch).toBeEnabled() + await launch.click() + await expect + .poll(async () => + page.evaluate(async () => { + const snapshot = (await window.electronAPI.invoke( + "workspace_snapshot", + )) as { + workspaces: Array<{ + id: string + source?: { gitRepository?: string } + }> + jobs: Record + } + const workspace = snapshot.workspaces.find( + (entry) => entry.id === "backend", + ) + return { + state: snapshot.jobs.backend?.state, + source: workspace?.source?.gitRepository, + } + }), + ) + .toEqual({ state: "succeeded", source }) + } finally { + await app?.close() + } + }) +} diff --git a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts index d4931d6f04..785d6c1b38 100644 --- a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts +++ b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts @@ -29,11 +29,11 @@ vi.mock("../analytics.js", () => ({ const { registerIpcHandlers } = await import("../ipc.js") function setup() { const jobs = new WorkspaceJobs() - let exit!: (code: number) => void + let exit!: (code: number, error?: unknown) => void let line!: (line: string, stream: "stdout") => void const cli = { setUnlockHandler: vi.fn(), - run: vi.fn(async () => ({ id: "task-1" })), + run: vi.fn(async (_args: string[]) => ({ id: "task-1" })), runRaw: vi.fn( async () => '{"kind":"status","schemaVersion":1,"phase":"building_image","state":"started"}', @@ -54,9 +54,13 @@ function setup() { url: pathToFileURL(join(__dirname, "../../renderer/index.html")).href, isDestroyed: () => false, } - const webContents = { send, mainFrame, isDestroyed: () => false } + const webContents = { send, mainFrame, isDestroyed: vi.fn(() => false) } const win = { webContents, isDestroyed: () => false } trustedEvent = { sender: webContents, senderFrame: mainFrame } + const createLogFile = vi.fn(() => "/tmp/log") + const closeLog = vi.fn(async () => {}) + const appendLog = vi.fn(() => true) + const getMainWindow = vi.fn(() => win) const actions = registerIpcHandlers({ cli, state: { @@ -65,22 +69,27 @@ function setup() { workspaceList: () => [], }, logStore: { - createLogFile: () => "/tmp/log", - appendLog: () => true, - closeLog: async () => {}, + createLogFile, + appendLog, + closeLog, onDrain: async () => {}, }, pty: { cancelFor: vi.fn(async () => {}) }, - getMainWindow: () => win, + getMainWindow, providerJobs: new ProviderJobs(), workspaceJobs: jobs, } as unknown as Parameters[0]) return { jobs, cli, + closeLog, + appendLog, + createLogFile, + getMainWindow, + webContents, send, actions, - exit: (code: number) => exit(code), + exit: (code: number, error?: unknown) => exit(code, error), line: (text: string) => line(text, "stdout"), } } @@ -181,6 +190,392 @@ describe("workspace lifecycle IPC", () => { await flush() expect(ctx.jobs.get("ws")?.state).toBe("succeeded") }) + it("finishes progress after an unexpected launch exception and permits retry", async () => { + const ctx = setup() + vi.spyOn(ctx.actions.tunnelProcesses, "set").mockImplementationOnce(() => { + throw new Error("unexpected follower registration error") + }) + invoke("workspace_up", { source: "ws", commandId: "first" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ state: "failed" }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ commandId: "first", success: false }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run).toHaveBeenCalledWith([ + "workspace", + "task", + "cancel", + "task-1", + ]) + }) + + it("finishes progress if creating the log fails before submission", async () => { + const ctx = setup() + ctx.createLogFile.mockImplementationOnce(() => { + throw new Error("log directory unavailable") + }) + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + expect(ctx.cli.run).not.toHaveBeenCalled() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: "failed", + error: "log directory unavailable", + }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ + success: false, + cliError: { message: "log directory unavailable" }, + }) + }) + + it("preserves preflight error and hint in one terminal event", async () => { + const ctx = setup() + ctx.cli.run.mockRejectedValueOnce( + Object.assign(new Error("invalid ID"), { + cliError: { + code: "invalid_id", + message: "invalid ID", + hint: "Use lowercase", + }, + }), + ) + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ + success: false, + cliError: { + code: "invalid_id", + message: "invalid ID", + hint: "Use lowercase", + }, + }) + expect(ctx.jobs.get("ws")).toMatchObject({ + state: "failed", + error: "invalid ID", + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + }) + + it("reports a persisted failed task after its follower disconnects", async () => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.cli.runRaw.mockRejectedValue(new Error("task failed")) + ctx.cli.run.mockResolvedValue([ + { + id: "task-1", + status: "failed", + error: "invalid ID", + errorCode: "invalid_id", + }, + ] as never) + ctx.exit(1) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: "failed", + error: "invalid ID", + }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ + success: false, + cliError: { message: "invalid ID" }, + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + ctx.cli.run.mockResolvedValue({ id: "task-2" }) + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run.mock.calls.at(-1)?.[0]).not.toContain("cancel") + }) + + it("ignores a stale persisted result after a new generation owns the workspace", async () => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "old" }) + await flush() + let resolve!: (value: string) => void + ctx.cli.runRaw.mockImplementationOnce( + () => + new Promise((done) => { + resolve = done + }), + ) + ctx.exit(1) + await flush() + ctx.jobs.clear("ws") + ctx.jobs.start("ws", "creating", "new") + resolve('{"kind":"result","outcome":"success"}') + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "new", + state: "running", + }) + expect( + ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ), + ).toHaveLength(0) + }) + + it.each(["success", "failure"])( + "keeps %s visible and unconfirmed when log close fails", + async (outcome) => { + const ctx = setup() + ctx.closeLog.mockRejectedValueOnce(new Error("disk unavailable")) + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.line( + outcome === "success" + ? '{"kind":"result","outcome":"success"}' + : '{"kind":"error","outcome":"error","code":"invalid_id","message":"invalid ID","hint":"Use lowercase"}', + ) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: outcome === "success" ? "reconciling" : "failed", + logError: "Workspace log could not be saved: disk unavailable", + ...(outcome === "failure" ? { error: "invalid ID" } : {}), + }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1].success).toBe(outcome === "success") + if (outcome === "failure") + expect(terminal[0][1].cliError).toMatchObject({ + message: "invalid ID", + hint: "Use lowercase", + }) + await ctx.jobs.retryRefresh("ws") + expect(ctx.jobs.get("ws")?.state).toBe( + outcome === "success" ? "reconciling" : "failed", + ) + expect(ctx.jobs.get("ws")?.logError).toBe( + "Workspace log could not be saved: disk unavailable", + ) + }, + ) + + it.each(["success", "failure"])( + "preserves terminal %s when renderer delivery throws", + async (outcome) => { + const ctx = setup() + vi.stubEnv("DEVSY_TEST_TOKEN", "private-value") + const unhandled: unknown[] = [] + const onUnhandled = (error: unknown) => unhandled.push(error) + process.on("unhandledRejection", onUnhandled) + try { + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.send.mockImplementation((channel, progress) => { + if (channel === "command-progress" && progress.done) + throw new Error("renderer delivery failed") + }) + ctx.line( + outcome === "success" + ? '{"kind":"result","outcome":"success"}' + : '{"kind":"error","outcome":"error","code":"invalid_id","message":"invalid ID token=private-value","hint":"Use lowercase"}', + ) + await flush() + await vi.advanceTimersByTimeAsync(0) + expect(unhandled).toEqual([]) + expect(ctx.jobs.get("ws")).toMatchObject({ + state: outcome === "success" ? "succeeded" : "failed", + ...(outcome === "failure" ? { error: "invalid ID token=***" } : {}), + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + if (outcome === "failure") { + const terminal = ctx.send.mock.calls.find( + ([channel, progress]) => + channel === "command-progress" && progress.done, + ) + expect(terminal?.[1].cliError).toMatchObject({ + message: "invalid ID token=***", + hint: "Use lowercase", + }) + } + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run).not.toHaveBeenCalledWith([ + "workspace", + "task", + "cancel", + "task-1", + ]) + } finally { + process.removeListener("unhandledRejection", onUnhandled) + vi.unstubAllEnvs() + } + }, + ) + + it.each(["destroyed contents", "window lookup throws"])( + "finishes the main job when %s prevents progress delivery", + async (boundary) => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + if (boundary === "destroyed contents") + ctx.webContents.isDestroyed.mockReturnValue(true) + else + ctx.getMainWindow.mockImplementation(() => { + throw new Error("window unavailable") + }) + ctx.line('{"kind":"result","outcome":"success"}') + await flush() + expect(ctx.jobs.get("ws")?.state).toBe("succeeded") + expect( + ctx.send.mock.calls.filter( + ([channel]) => channel === "command-progress", + ), + ).toHaveLength(0) + expect(ctx.closeLog).toHaveBeenCalledOnce() + }, + ) + + it.each(["success", "failure"])( + "keeps %s and task ownership after status delivery throws", + async (outcome) => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.send.mockImplementation((channel) => { + if (channel === "workspace-status") + throw new Error("status renderer unavailable") + }) + expect(() => + ctx.line( + '{"kind":"status","schemaVersion":1,"phase":"building_image","state":"started"}', + ), + ).not.toThrow() + expect(ctx.jobs.get("ws")).toMatchObject({ state: "running" }) + ctx.line( + outcome === "success" + ? '{"kind":"result","outcome":"success"}' + : '{"kind":"error","outcome":"error","code":"invalid_id","message":"invalid ID","hint":"Use lowercase"}', + ) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: outcome === "failure" ? "failed" : "succeeded", + ...(outcome === "failure" ? { error: "invalid ID" } : {}), + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + const terminal = ctx.send.mock.calls.filter( + ([channel, progress]) => + channel === "command-progress" && progress.done, + ) + expect(terminal).toHaveLength(1) + if (outcome === "failure") + expect(terminal[0][1].cliError).toMatchObject({ + code: "invalid_id", + message: "invalid ID", + hint: "Use lowercase", + }) + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run).not.toHaveBeenCalledWith([ + "workspace", + "task", + "cancel", + "task-1", + ]) + }, + ) + + it.each([0, 1])( + "retains action outcome %s when forwarded status delivery throws", + async (exitCode) => { + const ctx = setup() + invoke("workspace_rebuild") + await flush() + ctx.send.mockImplementation((channel) => { + if (channel === "workspace-status") + throw new Error("status renderer unavailable") + }) + expect(() => + ctx.line( + '{"kind":"status","schemaVersion":1,"phase":"building_image","state":"started"}', + ), + ).not.toThrow() + expect(ctx.jobs.get("ws")?.state).toBe("running") + ctx.exit(exitCode) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: exitCode === 0 ? "succeeded" : "failed", + ...(exitCode === 1 ? { error: "Command exited with code 1" } : {}), + }) + }, + ) + + it.each([0, 1])( + "keeps action outcome %s unconfirmed when log close fails", + async (exitCode) => { + const ctx = setup() + vi.stubEnv("TEST_API_TOKEN", "private-token") + ctx.closeLog.mockRejectedValueOnce(new Error("disk unavailable")) + invoke("workspace_rebuild") + await flush() + ctx.exit(exitCode, { + code: "action_failed", + message: "Cannot rebuild private-token", + }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: exitCode === 0 ? "reconciling" : "failed", + logError: "Workspace log could not be saved: disk unavailable", + error: exitCode === 0 ? undefined : "Cannot rebuild ***", + }) + const terminal = () => + ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal()).toHaveLength(1) + expect(terminal()[0][1].success).toBe(exitCode === 0) + if (exitCode !== 0) + expect(terminal()[0][1].cliError).toMatchObject({ + code: "action_failed", + message: "Cannot rebuild ***", + }) + await ctx.jobs.retryRefresh("ws") + expect(ctx.jobs.get("ws")).toMatchObject({ + state: exitCode === 0 ? "reconciling" : "failed", + logError: "Workspace log could not be saved: disk unavailable", + refreshError: undefined, + error: exitCode === 0 ? undefined : "Cannot rebuild ***", + }) + expect(terminal()).toHaveLength(1) + expect(ctx.closeLog).toHaveBeenCalledOnce() + }, + ) + it("uses the same registry for tray stop", async () => { const ctx = setup() const completion = ctx.actions.workspaceActions.stop("ws") diff --git a/desktop/src/main/__tests__/watcher.test.ts b/desktop/src/main/__tests__/watcher.test.ts index 2ac2a6f273..afd073f26f 100644 --- a/desktop/src/main/__tests__/watcher.test.ts +++ b/desktop/src/main/__tests__/watcher.test.ts @@ -10,6 +10,7 @@ function makeWatcher( ) { const state = { updateProviders: vi.fn().mockReturnValue(false), + updateWorkspaces: vi.fn().mockReturnValue(false), providerReadRevision: vi.fn().mockReturnValue(0), providerList: vi.fn().mockReturnValue([]), workspaceList: vi.fn().mockReturnValue(workspaces), @@ -330,3 +331,127 @@ describe("workspace status ordering", () => { await expect(watcher.refreshWorkspaceStatus("ws")).resolves.toBeUndefined() }) }) + +describe("Watcher job broadcasts", () => { + it.each( + ["success", "failure"].flatMap((outcome) => + ["send", "lookup", "window", "contents"].map((boundary) => [ + outcome, + boundary, + ]), + ), + )( + "keeps %s reconciliation independent of renderer %s failure", + async (outcome, boundary) => { + const state = new DaemonState() + const workspaceJobs = new WorkspaceJobs() + const send = vi.fn() + const win = { + isDestroyed: vi.fn(() => false), + webContents: { isDestroyed: vi.fn(() => false), send }, + } + const getMainWindow = vi.fn(() => win) + const cli = { + run: vi.fn(async () => [{ id: "ws", context: "default" }]), + runRaw: vi.fn(async () => JSON.stringify({ state: "running" })), + } + const watcher = new Watcher({ + cli: cli as never, + state, + getMainWindow: getMainWindow as never, + providerJobs: { snapshot: () => ({}) } as never, + workspaceJobs, + }) + workspaceJobs.onChange(() => watcher.broadcastWorkspaces()) + const refresh = vi.fn(async () => { + await watcher.refreshWorkspaces() + await watcher.refreshWorkspaceStatus("ws") + watcher.broadcastWorkspaces() + }) + workspaceJobs.setRefresh(refresh) + const generation = workspaceJobs.start("ws", "creating", "first") + send.mockClear() + if (boundary === "send") + send.mockImplementation(() => { + throw new Error("renderer delivery failed") + }) + if (boundary === "lookup") + getMainWindow.mockImplementation(() => { + throw new Error("window lookup failed") + }) + if (boundary === "window") win.isDestroyed.mockReturnValue(true) + if (boundary === "contents") + win.webContents.isDestroyed.mockReturnValue(true) + expect(() => + workspaceJobs.phase("ws", "first", "Launching command"), + ).not.toThrow() + expect(() => + workspaceJobs.progress("ws", "first", { + phase: "building_image", + state: "started", + }), + ).not.toThrow() + const originalError = outcome === "failure" ? "invalid ID" : undefined + await expect( + workspaceJobs.finish("ws", generation, originalError), + ).resolves.toBeUndefined() + expect(refresh).toHaveBeenCalledOnce() + expect(cli.run).toHaveBeenCalledOnce() + expect(cli.runRaw).toHaveBeenCalledOnce() + expect(workspaceJobs.get("ws")).toMatchObject({ + state: outcome === "failure" ? "failed" : "succeeded", + error: originalError, + refreshError: undefined, + }) + if (boundary === "window" || boundary === "contents") + expect(send).not.toHaveBeenCalled() + expect(() => workspaceJobs.start("ws", "creating", "retry")).not.toThrow() + expect(workspaceJobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + }, + ) +}) + +describe("Watcher log confirmation", () => { + it.each([undefined, "command denied"])( + "preserves log failures through polling and manual refresh (%s)", + async (error) => { + const { watcher, workspaceJobs } = makeWatcher(async () => ({})) + const refresh = vi + .fn() + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValue(undefined) + workspaceJobs.setRefresh(refresh) + const generation = workspaceJobs.start("ws", "creating", "first") + await workspaceJobs.finish( + "ws", + generation, + error, + "Workspace log could not be saved: disk unavailable", + ) + await (watcher as unknown as { pollOnce(): Promise }).pollOnce() + expect(refresh).not.toHaveBeenCalled() + await workspaceJobs.retryRefresh("ws") + expect(workspaceJobs.get("ws")?.refreshError).toBe("offline") + await (watcher as unknown as { pollOnce(): Promise }).pollOnce() + for (let i = 0; i < 20; i++) await Promise.resolve() + if (error) await workspaceJobs.retryRefresh("ws") + expect(refresh).toHaveBeenCalledTimes(2) + await workspaceJobs.retryRefresh("ws") + expect(workspaceJobs.get("ws")).toMatchObject({ + state: error ? "failed" : "reconciling", + logError: "Workspace log could not be saved: disk unavailable", + refreshError: undefined, + error, + }) + const next = workspaceJobs.start("ws", "creating", "retry") + expect(next).not.toBe(generation) + expect(workspaceJobs.get("ws")?.state).toBe("running") + expect(workspaceJobs.get("ws")?.logError).toBeUndefined() + await workspaceJobs.finish("ws", generation) + expect(workspaceJobs.get("ws")?.commandId).toBe("retry") + }, + ) +}) diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index ed6c5d3cd4..0f41d0f617 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -25,6 +25,7 @@ import type { MachineDiagnosticsManager } from "./machine-diagnostics-manager.js import type { MachineDiagnosticsStore } from "./machine-diagnostics-store.js" import type { ProviderActivity, ProviderJobs } from "./provider-jobs.js" import type { PtyManager } from "./pty.js" +import { sendRendererEvent } from "./renderer-events.js" import { isTrustedSecretIpcSender } from "./secret-ipc-policy.js" import { SecretSession } from "./secret-session.js" import type { SettingsService } from "./settings-service.js" @@ -192,7 +193,7 @@ interface ProgressSink { cliError?: CLIError success?: boolean }, - ): Promise + ): Promise } function redactSensitiveText(value: string): string { @@ -293,9 +294,7 @@ function createLogSink( if (!done && buf.length === 0) return const lines = buf buf = [] - const win = getWin() - if (!win || win.isDestroyed?.()) return - win.webContents.send("command-progress", { + sendRendererEvent(getWin, "command-progress", { commandId, lines: lines.map(redactSensitiveText), done, @@ -328,14 +327,21 @@ function createLogSink( finished = true const safeLine = redactSensitiveText(finalLine) buf.push(safeLine) + let flushError: string | undefined try { - appendLog?.(safeLine) - await flush?.() + try { + appendLog?.(safeLine) + } finally { + await flush?.() + } } catch (error) { - // Log persistence must not change a command's actual outcome. + flushError = redactSensitiveText( + `Workspace log could not be saved: ${error instanceof Error ? error.message : String(error)}`, + ) mainLog.warn("[workspace-operation] log flush failed", error) } post(true, { message: finalLine, ...extra }) + return flushError }, } } @@ -953,7 +959,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { if (!workspaceJobs.owns(workspaceId, commandId)) return true const status = redactOperationStatus(normalizeOperationStatus(envelope)) workspaceJobs.progress(workspaceId, commandId, status) - deps.getMainWindow()?.webContents.send("workspace-status", { + sendRendererEvent(deps.getMainWindow, "workspace-status", { commandId, workspaceId, ...status, @@ -2006,6 +2012,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { `[workspace-operation] ${cmdId} ${stage} ${Math.round(performance.now() - started)}ms`, ) timing("accepted") + let failureSink = createLogSink(deps.getMainWindow, cmdId) const completion = (async () => { const logPath = logStore.createLogFile(state.workspaceContext(wsId), wsId) const sink = createLogSink( @@ -2015,6 +2022,16 @@ export function registerIpcHandlers(deps: IpcDependencies): { () => logStore.closeLog(logPath), ) + failureSink = sink + const finishUp = async ( + line: string, + extra: Parameters[1], + error?: string, + ) => { + const flushError = await sink.done(line, extra) + await workspaceJobs.finish(wsId, generation, error, flushError) + } + await serializePerWorkspace(wsId, async () => { // Tear down any prior run for this workspace before starting a new // one. @@ -2035,21 +2052,15 @@ export function registerIpcHandlers(deps: IpcDependencies): { } taskId = submitted.id } catch (error) { - const err = error as Error & { cliError?: CLIError } - void sink.done(formatLogLine(err.message, "ERROR"), { - level: "error", - success: false, - cliError: err.cliError ?? { - code: "up_failed", - message: err.message, + const safe = redactCLIError(cliErrorOrFallback(error, "up_failed")) + await finishUp( + formatLogLine(safe.message, "ERROR"), + { + level: "error", + success: false, + cliError: safe, }, - }) - await workspaceJobs.finish( - wsId, - generation, - redactCLIError( - err.cliError ?? { code: "up_failed", message: err.message }, - ).message, + safe.message, ) return cmdId } @@ -2091,7 +2102,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { normalizeOperationStatus(envelope), ) workspaceJobs.progress(wsId, cmdId, status) - deps.getMainWindow()?.webContents.send("workspace-status", { + sendRendererEvent(deps.getMainWindow, "workspace-status", { commandId: cmdId, workspaceId: wsId, ...status, @@ -2105,32 +2116,28 @@ export function registerIpcHandlers(deps: IpcDependencies): { signalledDone = true releaseTask() timing("completed") - void workspaceJobs.finish(wsId, generation) - void sink.done(formatted, { success: true }) + void finishUp(formatted, { success: true }) return } if (envelope?.kind === "error") { signalledDone = true releaseTask() - void workspaceJobs.finish( - wsId, - generation, - redactCLIError({ - code: envelope.code ?? "up_failed", - message: envelope.message, - }).message, - ) - void sink.done(formatted, { - level: "error", - success: false, - cliError: { - code: envelope.code ?? "up_failed", - message: envelope.message, - hint: envelope.hint, - context: envelope.context, - }, + const safe = redactCLIError({ + code: envelope.code ?? "up_failed", + message: envelope.message, + hint: envelope.hint, + context: envelope.context, }) + void finishUp( + formatted, + { + level: "error", + success: false, + cliError: safe, + }, + safe.message, + ) return } @@ -2183,11 +2190,19 @@ export function registerIpcHandlers(deps: IpcDependencies): { return cmdId }) })().catch(async (error) => { - await workspaceJobs.finish( - wsId, - generation, - redactCLIError(cliErrorOrFallback(error, "up_failed")).message, - ) + const safe = redactCLIError(cliErrorOrFallback(error, "up_failed")) + let flushError: string | undefined + if (workspaceJobs.owns(wsId, cmdId)) { + flushError = await failureSink.done( + formatLogLine(safe.message, "ERROR"), + { + success: false, + level: "error", + cliError: safe, + }, + ) + } + await workspaceJobs.finish(wsId, generation, safe.message, flushError) }) return { commandId: cmdId, completion } } @@ -2226,12 +2241,10 @@ export function registerIpcHandlers(deps: IpcDependencies): { const envelope = parseCliEnvelope(line) if (envelope?.kind === "result") { release() - await sink - .done(formatLogLine("Completed"), { success: true }) - .catch((error) => - mainLog.warn("[workspace-operation] log flush failed", error), - ) - await workspaceJobs.finish(wsId, generation) + const flushError = await sink.done(formatLogLine("Completed"), { + success: true, + }) + await workspaceJobs.finish(wsId, generation, undefined, flushError) return } if (envelope?.kind === "status") @@ -2263,11 +2276,19 @@ export function registerIpcHandlers(deps: IpcDependencies): { code: task.errorCode ?? "up_failed", message: task.error || "Workspace task failed", }) - await sink.done(formatLogLine(safe.message, "ERROR"), { - success: false, - cliError: safe, - }) - await workspaceJobs.finish(wsId, generation, safe.message) + const flushError = await sink.done( + formatLogLine(safe.message, "ERROR"), + { + success: false, + cliError: safe, + }, + ) + await workspaceJobs.finish( + wsId, + generation, + safe.message, + flushError, + ) return } } catch { @@ -2370,29 +2391,35 @@ export function registerIpcHandlers(deps: IpcDependencies): { .catch(reject) }) timing("completed") - await sink + const flushError = await sink .done(formatLogLine("Completed"), { success: true }) - .catch((error) => - mainLog.warn("[workspace-operation] log flush failed", error), - ) - await workspaceJobs.finish(args.workspaceId, generation) + .catch((error) => { + mainLog.warn("[workspace-operation] log flush failed", error) + return undefined + }) + await workspaceJobs.finish( + args.workspaceId, + generation, + undefined, + flushError, + ) timing("reconciled") } catch (error) { const cliError = redactCLIError( cliErrorOrFallback(error, "workspace_operation_failed"), ) - if (sink) - await sink - .done(formatLogLine(cliError.message, "ERROR"), { - success: false, - level: "error", - cliError, - }) - .catch(() => {}) + const flushError = await sink + ?.done(formatLogLine(cliError.message, "ERROR"), { + success: false, + level: "error", + cliError, + }) + .catch(() => undefined) await workspaceJobs.finish( args.workspaceId, generation, cliError.message, + flushError, ) throw error } diff --git a/desktop/src/main/renderer-events.ts b/desktop/src/main/renderer-events.ts new file mode 100644 index 0000000000..4fb81c7135 --- /dev/null +++ b/desktop/src/main/renderer-events.ts @@ -0,0 +1,17 @@ +import type { BrowserWindow } from "electron" +import { mainLog } from "./logging.js" + +export function sendRendererEvent( + getWindow: () => BrowserWindow | null, + channel: string, + payload: unknown, +): void { + try { + const window = getWindow() + if (!window || window.isDestroyed?.() || window.webContents.isDestroyed?.()) + return + window.webContents.send(channel, payload) + } catch { + mainLog.warn(`[renderer] ${channel} delivery unavailable`) + } +} diff --git a/desktop/src/main/watcher.ts b/desktop/src/main/watcher.ts index 6a8e9b0c3d..8dde979e41 100644 --- a/desktop/src/main/watcher.ts +++ b/desktop/src/main/watcher.ts @@ -7,6 +7,7 @@ import type { CliRunner } from "./cli.js" import type { DaemonClient } from "./daemon-client.js" import { mainLog } from "./logging.js" import type { ProviderJobs } from "./provider-jobs.js" +import { sendRendererEvent } from "./renderer-events.js" import type { DaemonState, Machine, Workspace } from "./state.js" import type { WorkspaceJobs } from "./workspace-jobs.js" import { normalizeWorkspaceStatus } from "./workspace-status.js" @@ -381,9 +382,6 @@ export class Watcher { } private send(channel: string, payload: unknown): void { - const win = this.deps.getMainWindow() - if (win && !win.isDestroyed()) { - win.webContents.send(channel, payload) - } + sendRendererEvent(this.deps.getMainWindow, channel, payload) } } diff --git a/desktop/src/main/workspace-jobs.ts b/desktop/src/main/workspace-jobs.ts index 3260cc6335..62686ee7b3 100644 --- a/desktop/src/main/workspace-jobs.ts +++ b/desktop/src/main/workspace-jobs.ts @@ -79,27 +79,33 @@ export class WorkspaceJobs { this.generations.set(id, ++this.lastGeneration) if (this.jobs.delete(id)) this.emit() } - async finish(id: string, generation: number, error?: string): Promise { + async finish( + id: string, + generation: number, + error?: string, + logError?: string, + ): Promise { const job = this.jobs.get(id) if (!job || this.generation(id) !== generation || job.state !== "running") return this.generations.set(id, ++this.lastGeneration) // invalidate pre-completion polls this.jobs.set(id, { ...job, - state: "reconciling", + state: error && logError ? "failed" : "reconciling", + logError, phase: job.activity === "deleting" ? "Refreshing list" : "Refreshing status", error, }) this.emit() - await this.retryRefresh(id) + if (!logError) await this.retryRefresh(id) } async retryRefresh(id: string): Promise { const job = this.jobs.get(id) if ( !job || (job.state !== "reconciling" && - !(job.state === "failed" && job.refreshError)) || + !(job.state === "failed" && (job.refreshError || job.logError))) || this.refreshing.has(job.commandId) ) return @@ -109,7 +115,11 @@ export class WorkspaceJobs { if (this.jobs.get(id) !== job) return this.jobs.set(id, { ...job, - state: job.error ? "failed" : "succeeded", + state: job.error + ? "failed" + : job.logError + ? "reconciling" + : "succeeded", phase: job.error ? "See workspace logs for details" : "", refreshError: undefined, }) diff --git a/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte b/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte index ecd572263d..1f8461a42f 100644 --- a/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte +++ b/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte @@ -1,4 +1,5 @@ @@ -1326,7 +1305,7 @@ function selectTemplate(t: { name: string; source: string }) { - Please provide an explicit workspace name. The derived id is empty or invalid. + {workspaceIdRule} Try "{deriveWorkspaceId(resolvedId)}". {/if} @@ -1475,6 +1454,7 @@ function selectTemplate(t: { name: string; source: string }) { running={launchRunning || launchConfirmationState === "confirming"} error={launchError || undefined} confirmationState={launchConfirmationState} + logError={launchLogError} refreshError={launchStatusRefreshError ?? undefined} refreshRetrying={launchRefreshRetrying} onRetryStatus={retryLaunchStatus} @@ -1510,9 +1490,9 @@ function selectTemplate(t: { name: string; source: string }) { {/if} - {#if launchError || launchSuccess || launchStatusRefreshError} + {#if launchError || launchSuccess || launchStatusRefreshError || launchLogError}
- {#if !launchError && (launchSuccess || launchStatusRefreshError)} + {#if !launchError && (launchSuccess || launchStatusRefreshError || launchLogError)} + {#if launchLogError}{/if} {:else if launchError} {#if launchBuildFailed && !launchIsRecovery} diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts index 8b61475962..bfef3c592f 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts @@ -382,6 +382,41 @@ describe("WorkspaceWizard", () => { unmount() }) + it.each([ + ["Local Directory", "/var/home/dev/Projects/Backend", "Local Directory *"], + ["Git Repository", "https://example.com/Backend.git", "Repository URL *"], + ])( + "derives a valid ID from %s without changing the source", + async (tab, source, label) => { + providers.set([makeProvider("docker")]) + const { getByText, getByLabelText, unmount } = render(WorkspaceWizard, { + props: { open: true }, + }) + await flushAsync() + await fireEvent.click(getByText("docker")) + await fireEvent.click(getActiveContinue(getByText)) + await flushAsync() + if (tab === "Local Directory") + await fireEvent.click(getByText("Local Directory")) + await fireEvent.input(getByLabelText(label), { + target: { value: source }, + }) + await fireEvent.click(getActiveContinue(getByText)) + await flushAsync() + await fireEvent.click(getActiveContinue(getByText)) + await flushAsync() + expect( + (document.querySelector("#workspace-name") as HTMLInputElement).value, + ).toBe("backend") + await fireEvent.click(getLaunchButton()) + await flushAsync() + expect(workspaceUp).toHaveBeenCalledWith( + expect.objectContaining({ workspaceId: "backend", source }), + ) + unmount() + }, + ) + it("shows the image picker when the Image tab is selected", async () => { providers.set([makeProvider("docker")]) const { getByText, unmount } = render(WorkspaceWizard, { @@ -432,6 +467,32 @@ describe("WorkspaceWizard", () => { unmount() }) + it.each(["Backend", "back_end", "back.end", "back end", "a".repeat(49)])( + "blocks invalid explicit ID %s with an accessible rule", + async (id) => { + providers.set([makeProvider("docker")]) + const { getByText, unmount } = render(WorkspaceWizard, { + props: { open: true }, + }) + await flushAsync() + await advanceToReview(getByText) + const input = document.querySelector( + "#workspace-name", + ) as HTMLInputElement + await fireEvent.input(input, { target: { value: id } }) + await flushAsync() + expect(input.value).toBe(id) + expect(input.getAttribute("aria-invalid")).toBe("true") + expect(input.getAttribute("aria-describedby")).toBe( + "workspace-name-invalid", + ) + expect(getByText(/1-48 lowercase letters/)).toBeTruthy() + expect(getLaunchButton().disabled).toBe(true) + expect(workspaceUp).not.toHaveBeenCalled() + unmount() + }, + ) + it("blocks Launch when the user edits the name into a conflict", async () => { providers.set([makeProvider("docker")]) workspaces.set([{ id: "python" }]) @@ -687,6 +748,33 @@ describe("WorkspaceWizard", () => { unmount() }) + it("keeps a log failure unconfirmed without a status spinner or completion", async () => { + providers.set([makeProvider("docker")]) + const oncomplete = vi.fn() + const ui = render(WorkspaceWizard, { props: { open: true, oncomplete } }) + await flushAsync() + await advanceToReview(ui.getByText) + await fireEvent.click(getLaunchButton()) + await flushAsync() + ;(workspaceJobs as { set: (v: unknown) => void }).set({ + python: { + commandId: "cmd-1", + activity: "creating", + state: "reconciling", + phase: "", + logError: "Workspace log could not be saved: disk unavailable", + }, + }) + await flushAsync() + expect(ui.getByText(/Workspace log could not be saved/)).toBeTruthy() + expect(ui.queryByText("Workspace ready")).toBeNull() + expect(ui.queryByText("Confirming workspace status")).toBeNull() + expect(ui.queryByRole("button", { name: "Retry status" })).toBeNull() + expect(ui.getByRole("button", { name: /^Retry$/ })).toBeTruthy() + expect(oncomplete).not.toHaveBeenCalled() + ui.unmount() + }) + it("keeps a refresh-only failure stale until Retry status confirms the workspace", async () => { providers.set([makeProvider("docker")]) const oncomplete = vi.fn() @@ -1035,7 +1123,7 @@ describe("WorkspaceWizard", () => { const nameInput = document.querySelector( 'input[placeholder*="derived from source"]', ) as HTMLInputElement - expect(nameInput.value).toBe("ubuntu-22.04") + expect(nameInput.value).toBe("ubuntu-22-04") const launchBtn = Array.from(document.querySelectorAll("button")).find( (b) => b.textContent?.trim() === "Launch", diff --git a/desktop/src/renderer/src/lib/stores/workspaces.test.ts b/desktop/src/renderer/src/lib/stores/workspaces.test.ts index c0525123d7..2b388319dc 100644 --- a/desktop/src/renderer/src/lib/stores/workspaces.test.ts +++ b/desktop/src/renderer/src/lib/stores/workspaces.test.ts @@ -336,6 +336,50 @@ describe("workspace snapshot store", () => { expect(toasts.success).not.toHaveBeenCalled() expect(toasts.error).not.toHaveBeenCalled() }) + it("does not toast ready while log persistence remains unconfirmed", async () => { + mockInvoke.mockResolvedValue({ + revision: 1, + workspaces: [{ id: "ws", status: "Running" }], + jobs: { ws: job }, + }) + await initWorkspaces() + for (const revision of [2, 3]) + event({ + revision, + workspaces: [{ id: "ws", status: "Running" }], + jobs: { + ws: { + ...job, + state: "reconciling", + logError: "Workspace log could not be saved: disk unavailable", + }, + }, + }) + expect(toasts.success).not.toHaveBeenCalled() + expect(toasts.error).not.toHaveBeenCalled() + }) + it("retains a deleted workspace row while its log warning is unresolved", async () => { + mockInvoke.mockResolvedValue({ + revision: 1, + workspaces: [{ id: "ws", status: "Running" }], + jobs: { ws: job }, + }) + await initWorkspaces() + event({ + revision: 2, + workspaces: [], + jobs: { + ws: { + ...job, + activity: "deleting", + state: "reconciling", + logError: "Workspace log could not be saved: disk unavailable", + }, + }, + }) + expect(get(workspaces).map((workspace) => workspace.id)).toEqual(["ws"]) + expect(toasts.success).not.toHaveBeenCalled() + }) it("leaves existing observations intact when snapshot loading fails", async () => { workspaces.set([{ id: "ws", status: "Running" }]) mockInvoke.mockRejectedValue(new Error("offline")) diff --git a/desktop/src/renderer/src/lib/stores/workspaces.ts b/desktop/src/renderer/src/lib/stores/workspaces.ts index 6fb44bb291..73d7209e70 100644 --- a/desktop/src/renderer/src/lib/stores/workspaces.ts +++ b/desktop/src/renderer/src/lib/stores/workspaces.ts @@ -50,6 +50,7 @@ function apply( ([id, job]) => !updated.some((workspace) => workspace.id === id) && (job.state === "running" || + !!job.logError || (job.activity === "creating" && job.state !== "succeeded")), ) workspaces.set([...updated, ...pending.map(([id]) => ({ id }))]) diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.test.ts b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts new file mode 100644 index 0000000000..ec190d8b88 --- /dev/null +++ b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from "vitest" +import { + deriveWorkspaceId, + isValidWorkspaceId, + uniqueWorkspaceId, +} from "./workspace-id.js" + +describe("workspace IDs", () => { + it.each(["backend", "backend-name-1", "a".repeat(48)])("accepts %s", (id) => { + expect(isValidWorkspaceId(id)).toBe(true) + }) + it.each(["", "Backend", "back_end", "back.end", "back end", "a".repeat(49)])( + "rejects %s", + (id) => { + expect(isValidWorkspaceId(id)).toBe(false) + }, + ) + it.each([ + "/var/home/dev/Projects/Backend", + "/home/dev/Backend/", + "/home/dev/@work/Backend//", + "/home/first@corp/Backend/", + "C:\\Users\\dev\\Backend\\", + "C:\\Users\\first@corp\\Backend\\", + "/home/dev/@work/Backend", + "/home/first@corp/Backend", + "C:\\Users\\first@corp\\Backend", + "./first@corp/Backend", + "../first@corp/Backend", + "~/first@corp/Backend", + "https://example.com/Backend.git", + "ssh://git@host/org/Backend.git@main", + "https://user@example.com/org/Backend.git@sha256:abc123", + "git@host:org/Backend.git@feature/foo@subpath:sub/path", + "github.com/org/Backend@pull/123/head", + "file:///projects/Backend.git@main", + "https://example.com/Backend.git@feature/foo", + "git@host:org/Backend.git", + "github.com/org/Backend@main@subpath:sub/path", + "github.com/org/Backend@main:sub/path", + ])("derives backend from %s without changing the source", (source) => { + const original = source + expect(deriveWorkspaceId(source)).toBe("backend") + expect(source).toBe(original) + }) + it.each([ + "/home/dev/Backend@work", + "C:\\Users\\dev\\Backend@work", + "Backend@work", + ])("preserves the local basename in %s", (source) => { + expect(deriveWorkspaceId(source)).toBe("backend-work") + }) + it("normalizes, truncates and falls back to valid IDs", () => { + for (const source of ["a".repeat(80), "___", "App.Name With Spaces"]) + expect(isValidWorkspaceId(deriveWorkspaceId(source))).toBe(true) + }) + it("preserves case insensitive collisions within the length limit", () => { + const base = "a".repeat(48) + const id = uniqueWorkspaceId(base, [base.toUpperCase()]) + expect(id).toBe(`${"a".repeat(46)}-1`) + expect(isValidWorkspaceId(id)).toBe(true) + }) +}) diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.ts b/desktop/src/renderer/src/lib/utils/workspace-id.ts new file mode 100644 index 0000000000..63386c1063 --- /dev/null +++ b/desktop/src/renderer/src/lib/utils/workspace-id.ts @@ -0,0 +1,40 @@ +export const workspaceIdRule = + "Use 1-48 lowercase letters (a-z), numbers, or dashes." + +export function isValidWorkspaceId(id: string): boolean { + return /^[a-z0-9-]{1,48}$/.test(id) +} + +export function deriveWorkspaceId(source: string): string { + const trimmed = source.trim() + const isRepository = + /^[a-z][a-z0-9+.-]*:\/\//i.test(trimmed) || + /^[^/@\\]+@[^:/\\]+:/.test(trimmed) || + /^[^/@\\]+\.[^/@\\]+\//.test(trimmed) + const repository = isRepository + ? trimmed + .replace(/^[^/@]+@[^:]+:/, "") + .replace(/^[a-z][a-z0-9+.-]*:\/\/[^/]*\//i, "") + .split("@")[0] + : trimmed + const leaf = repository.split(/[\\/]/).filter(Boolean).pop() ?? "" + return ( + leaf + .replace(/\.git$/i, "") + .toLowerCase() + .replace(/[^a-z0-9-]+/g, "-") + .replace(/^-+|-+$/g, "") + .slice(0, 48) + .replace(/-+$/g, "") || "workspace" + ) +} + +export function uniqueWorkspaceId(base: string, ids: string[]): string { + const existing = new Set(ids.map((id) => id.toLowerCase())) + if (!existing.has(base.toLowerCase())) return base + for (let i = 1; ; i++) { + const suffix = `-${i}` + const candidate = `${base.slice(0, 48 - suffix.length)}${suffix}` + if (!existing.has(candidate.toLowerCase())) return candidate + } +} diff --git a/desktop/src/shared/workspace-operation.ts b/desktop/src/shared/workspace-operation.ts index d4f05a9abf..151265c440 100644 --- a/desktop/src/shared/workspace-operation.ts +++ b/desktop/src/shared/workspace-operation.ts @@ -14,12 +14,14 @@ export interface WorkspaceJob { phase: string status?: OperationStatus error?: string + logError?: string refreshError?: string } export function workspaceJobBusy(job?: WorkspaceJob): boolean { return ( - job?.state === "running" || (job?.state === "reconciling" && !job.error) + job?.state === "running" || + (job?.state === "reconciling" && !job.error && !job.logError) ) } @@ -153,6 +155,16 @@ export function presentWorkspaceStatus(input: { } const headline = HEADLINE[job.activity] if (job.state === "reconciling") { + if (job.logError) { + return { + headline, + tone: "warning", + busy: false, + announce: `${headline}. ${job.logError}`, + recovery: { message: job.logError, canRetry: !!job.refreshError }, + detailsAvailable: true, + } + } if (job.refreshError) { const message = stalledMessage(job.activity) return { diff --git a/e2e/tests/ssh/detached_validation.go b/e2e/tests/ssh/detached_validation.go new file mode 100644 index 0000000000..e88b77f9a3 --- /dev/null +++ b/e2e/tests/ssh/detached_validation.go @@ -0,0 +1,269 @@ +package ssh + +import ( + "context" + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/task" + "github.com/devsy-org/devsy/pkg/workspace" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe("detached workspace input validation", ginkgo.Label("task-cancel"), func() { + ginkgo.It( + "rejects invalid explicit IDs without submitting tasks", + ginkgo.SpecTimeout(time.Minute), + func(ctx context.Context) { + wd, err := os.Getwd() + framework.ExpectNoError(err) + f := framework.NewDefaultFramework(filepath.Join(wd, "bin")) + home, err := os.MkdirTemp("", "devsy-invalid-id-") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(os.RemoveAll, home) + cli := detachedValidationCLI{framework: f, home: home} + for _, id := range []string{"Backend", "backend_name", "backend.api", "has spaces", strings.Repeat("a", 49)} { + out, err := cli.command(ctx, + cmdWorkspace, + "up", + "--detach", + "--result-format=json", + "--id", + id, + ) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(out).To(gomega.ContainSubstring("workspace name")) + gomega.Expect(out).NotTo(gomega.ContainSubstring(`"kind":"task"`)) + } + out, err := cli.command(ctx, cmdWorkspace, "task", "list") + framework.ExpectNoError(err) + var states []detachedUpState + framework.ExpectNoError(json.Unmarshal([]byte(out), &states)) + gomega.Expect(states).To(gomega.BeEmpty()) + }, + ) + + ginkgo.It( + "records early provider failure and preserves a mixed-case local source", + ginkgo.SpecTimeout(time.Minute), + func(ctx context.Context) { + wd, err := os.Getwd() + framework.ExpectNoError(err) + f := framework.NewDefaultFramework(filepath.Join(wd, "bin")) + home, err := os.MkdirTemp("", "devsy-early-worker-") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(os.RemoveAll, home) + cli := detachedValidationCLI{framework: f, home: home} + source := filepath.Join(home, "Backend.API") + framework.ExpectNoError(os.MkdirAll(source, 0o700)) + sentinel := filepath.Join(source, "KeepCase.txt") + framework.ExpectNoError(os.WriteFile(sentinel, []byte("original source"), 0o600)) + out, err := cli.command(ctx, + cmdWorkspace, + "up", + "--detach", + "--result-format=json", + "--provider=missing-provider-1471", + source, + ) + framework.ExpectNoError(err) + var envelope struct { + Kind string `json:"kind"` + ID string `json:"id"` + } + framework.ExpectNoError(json.Unmarshal([]byte(strings.TrimSpace(out)), &envelope)) + gomega.Expect(envelope.Kind).To(gomega.Equal("task")) + var states []struct { + ID string `json:"id"` + Status string `json:"status"` + WorkspaceID string `json:"workspaceId"` + Error string `json:"error"` + } + gomega.Eventually(func() string { + out, err := cli.command(ctx, cmdWorkspace, "task", "list") + if err != nil { + return err.Error() + } + if err := json.Unmarshal([]byte(out), &states); err != nil { + return err.Error() + } + for _, state := range states { + if state.ID == envelope.ID { + return state.Status + } + } + return "missing" + }).WithContext(ctx).WithTimeout(20 * time.Second).WithPolling(100 * time.Millisecond).Should(gomega.Equal("failed")) + gomega.Expect(states).To(gomega.HaveLen(1)) + gomega.Expect(states[0].WorkspaceID).To(gomega.Equal(workspace.ToID(source))) + gomega.Expect(states[0].Error).To(gomega.ContainSubstring("prepare workspace client")) + gomega.Expect(states[0].Error).NotTo(gomega.ContainSubstring("missing-provider-1471")) + cli.expectFailureEnvelope(ctx, envelope.ID, "prepare workspace client") + sourceRoot, err := os.OpenRoot(source) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(sourceRoot.Close) + contents, err := sourceRoot.ReadFile("KeepCase.txt") + framework.ExpectNoError(err) + gomega.Expect(string(contents)).To(gomega.Equal("original source")) + entries, err := os.ReadDir(home) + framework.ExpectNoError(err) + var entryNames []string + for _, entry := range entries { + entryNames = append(entryNames, entry.Name()) + } + gomega.Expect(entryNames).To(gomega.ContainElement("Backend.API")) + gomega.Expect(entryNames).NotTo(gomega.ContainElement("backend.api")) + }, + ) +}) + +type detachedValidationCLI struct { + framework *framework.Framework + home string +} + +func (cli detachedValidationCLI) newCommand(ctx context.Context, args ...string) *exec.Cmd { + // #nosec G204 -- repository-built CLI fixture with spec-controlled arguments and temporary paths. + cmd := exec.CommandContext( + ctx, + filepath.Join(cli.framework.DevsyBinDir, cli.framework.DevsyBinName), + args...) + cmd.Env = detachedValidationEnvironment(cli.home) + cmd.WaitDelay = time.Second + return cmd +} + +func (cli detachedValidationCLI) command(ctx context.Context, args ...string) (string, error) { + out, err := cli.newCommand(ctx, args...).CombinedOutput() + return string(out), err +} + +var _ = ginkgo.Describe( + "detached workspace early worker errors", + ginkgo.Label("task-cancel"), + func() { + for _, stage := range []string{"config", "client"} { + ginkgo.It( + "persists early "+stage+" failure for JSON followers", + ginkgo.SpecTimeout(time.Minute), + func(ctx context.Context) { + wd, err := os.Getwd() + framework.ExpectNoError(err) + f := framework.NewDefaultFramework(filepath.Join(wd, "bin")) + home, err := os.MkdirTemp("", "devsy-worker-error-") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(os.RemoveAll, home) + ginkgo.GinkgoT().Setenv(config.EnvHome, home) + config.ResetPathManager() + ginkgo.DeferCleanup(config.ResetPathManager) + cli := detachedValidationCLI{framework: f, home: home} + store, err := task.NewStoreAt(filepath.Join(home, "state", "tasks")) + framework.ExpectNoError(err) + tk, err := store.CreateLaunch(task.CreateOptions{Command: "up"}) + framework.ExpectNoError(err) + expected := "prepare workspace client" + args := []string{ + cmdWorkspace, + "up", + "--task-id", + tk.ID(), + "--result-format=json", + "--id=Backend", + } + if stage == "config" { + framework.ExpectNoError( + os.WriteFile( + filepath.Join(home, "config.yaml"), + []byte("contexts: ["), + 0o600, + ), + ) + expected = "load devsy config" + } + ref, err := cli.launchWorker(ctx, tk, args...) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(tk.Cancel) + gomega.Eventually(func() (bool, error) { + return command.ProcessExited(ref) + }).WithContext(ctx).WithTimeout(10 * time.Second).Should(gomega.BeTrue()) + state, err := store.Get(tk.ID()) + framework.ExpectNoError(err) + gomega.Expect(state.Status).To(gomega.Equal(task.StatusFailed)) + gomega.Expect(state.Error).To(gomega.ContainSubstring(expected)) + cli.expectFailureEnvelope(ctx, tk.ID(), expected) + }, + ) + } + }, +) + +func (cli detachedValidationCLI) launchWorker( + ctx context.Context, + tk *task.Task, + args ...string, +) (command.ProcessRef, error) { + var ref command.ProcessRef + err := command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: task.WorkerProcessName(tk.ID()), + Lease: tk.LaunchLease(), + AtomicOwnership: true, + OnPrepared: tk.PrepareProcessOwnership, + OnStarted: func(started command.ProcessRef) error { + ref = started + if err := tk.PublishProcess(started); err != nil { + return err + } + commitCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + return tk.CommitLaunch(commitCtx) + }, + }, func() (*exec.Cmd, error) { + // Atomic Windows launch requires a plain command with file streams. + // #nosec G204 -- repository-built CLI fixture with spec-controlled arguments. + worker := exec.Command( + filepath.Join(cli.framework.DevsyBinDir, cli.framework.DevsyBinName), + args...) + worker.Env = detachedValidationEnvironment(cli.home) + return worker, nil + }) + if err != nil { + return ref, errors.Join(err, tk.FailLaunch(err)) + } + return ref, nil +} + +func (cli detachedValidationCLI) expectFailureEnvelope(ctx context.Context, id, diagnostic string) { + followCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + out, err := cli.command(followCtx, + cmdWorkspace, + "task", + "logs", + id, + "--follow", + "--result-format=json", + ) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(followCtx.Err()).NotTo(gomega.HaveOccurred()) + var envelope struct { + Kind string `json:"kind"` + Message string `json:"message"` + } + framework.ExpectNoError(json.Unmarshal([]byte(strings.TrimSpace(out)), &envelope)) + gomega.Expect(envelope.Kind).To(gomega.Equal("error")) + gomega.Expect(envelope.Message).To(gomega.ContainSubstring(diagnostic)) + gomega.Expect(out).NotTo(gomega.ContainSubstring("missing-provider-1471")) +} + +func detachedValidationEnvironment(home string) []string { + return append(os.Environ(), "DEVSY_HOME="+home, "DEVSY_TEST_TOKEN=missing-provider-1471") +} diff --git a/e2e/tests/ssh/proxy_command.go b/e2e/tests/ssh/proxy_command.go index c9b96550a0..edcf43ebae 100644 --- a/e2e/tests/ssh/proxy_command.go +++ b/e2e/tests/ssh/proxy_command.go @@ -3,13 +3,16 @@ package ssh import ( "bytes" "context" + "fmt" "os" "os/exec" "path/filepath" + "strconv" "strings" "time" "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/secrets" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" ) @@ -74,6 +77,18 @@ var _ = ginkgo.Describe( sshPath, err := exec.LookPath("ssh.exe") framework.ExpectNoError(err) host := filepath.Base(tempDir) + ".devsy" + traceDir := ginkgo.GinkgoT().TempDir() + traceRoot, err := os.OpenRoot(traceDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(traceRoot.Close) + tracePath := filepath.Join(traceDir, proxyCommandTraceFile) + traceFile, err := traceRoot.OpenFile( + proxyCommandTraceFile, + os.O_CREATE|os.O_EXCL|os.O_WRONLY, + 0o600, + ) + framework.ExpectNoError(err) + framework.ExpectNoError(traceFile.Close()) sshCtx, cancelSSH := context.WithTimeout(ctx, 30*time.Second) defer cancelSSH() // #nosec G204 -- controlled OpenSSH invocation for the E2E test @@ -81,6 +96,7 @@ var _ = ginkgo.Describe( sshCtx, sshPath, "-F", sshConfigPath, + "-vvv", "-E", tracePath, "-o", "BatchMode=yes", host, "printf", @@ -89,12 +105,31 @@ var _ = ginkgo.Describe( var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr + started := time.Now() err = cmd.Run() - framework.ExpectNoError( - err, - "OpenSSH should launch ProxyCommand; stdout=%q stderr=%q", - stdout.String(), stderr.String(), + elapsed := time.Since(started) + sshErr, cause, parentErr := sshCtx.Err(), context.Cause(sshCtx), ctx.Err() + exitCode := -1 + if cmd.ProcessState != nil { + exitCode = cmd.ProcessState.ExitCode() + } + diagnostic := fmt.Sprintf( + "started=%s elapsed=%s sshContext=%v cause=%v parentContext=%v exitCode=%d; %s", + started.UTC(). + Format(time.RFC3339Nano), + elapsed, + sshErr, + cause, + parentErr, + exitCode, + proxyCommandOutputDiagnostic(traceRoot, stdout.String(), stderr.String()), + ) + ginkgo.AddReportEntry( + "OpenSSH ProxyCommand diagnostic", + diagnostic, + ginkgo.ReportEntryVisibilityFailureOrVerbose, ) + framework.ExpectNoError(err, "OpenSSH should launch ProxyCommand; %s", diagnostic) gomega.Expect(strings.TrimSpace(stdout.String())).To( gomega.Equal("proxy-command-ok"), ) @@ -102,3 +137,31 @@ var _ = ginkgo.Describe( ) }, ) + +func proxyCommandOutputDiagnostic(traceRoot *os.Root, stdout, stderr string) string { + trace, err := traceRoot.ReadFile(proxyCommandTraceFile) + redactor := secrets.NewEnvironmentRedactor(os.Environ()) + return fmt.Sprintf( + "stdout=%s stderr=%s traceReadError=%s trace=%s", + proxyCommandExcerpt(redactor, stdout), + proxyCommandExcerpt(redactor, stderr), + proxyCommandExcerpt( + redactor, + fmt.Sprint(err), + ), + proxyCommandExcerpt(redactor, string(trace)), + ) +} + +const ( + proxyCommandTraceFile = "ssh trace.log" + proxyCommandExcerptLimit = 8192 +) + +func proxyCommandExcerpt(redactor *secrets.Redactor, text string) string { + quoted := strconv.QuoteToASCII(redactor.Redact(text)) + if len(quoted) <= proxyCommandExcerptLimit { + return quoted + } + return "[truncated]" + quoted[len(quoted)-proxyCommandExcerptLimit:] +} diff --git a/e2e/tests/ssh/proxy_command_test.go b/e2e/tests/ssh/proxy_command_test.go new file mode 100644 index 0000000000..3fd2702729 --- /dev/null +++ b/e2e/tests/ssh/proxy_command_test.go @@ -0,0 +1,27 @@ +package ssh + +import ( + "strings" + "testing" + + "github.com/devsy-org/devsy/pkg/secrets" + "github.com/stretchr/testify/require" +) + +func TestProxyCommandExcerptProtectsFailureReport(t *testing.T) { + secret := strings.Repeat("test-value-", 8) + redactor := secrets.NewEnvironmentRedactor([]string{"DEVSY_TOKEN=" + secret}) + input := strings.Repeat( + "padding", + 3000, + ) + "\x1b[31m\n" + secret + " https://user:password@example.com/end" + got := proxyCommandExcerpt(redactor, input) + require.LessOrEqual(t, len(got), proxyCommandExcerptLimit+len("[truncated]")) + require.NotContains(t, got, secret) + require.NotContains(t, got, "password") + require.NotContains(t, got, "\x1b") + require.NotContains(t, got, "\n") + require.Contains(t, got, `\x1b`) + require.Contains(t, got, "example.com/end") + require.Contains(t, got, "***") +} diff --git a/hack/ci/suites.yaml b/hack/ci/suites.yaml index 337b874f9b..83b3856940 100644 --- a/hack/ci/suites.yaml +++ b/hack/ci/suites.yaml @@ -38,7 +38,13 @@ unit: - name: e2e-runtime-helpers description: E2E runtime helper packages - packages: [./e2e/framework/runenv/..., ./e2e/framework/dockerwsl, ./e2e/framework/ginkgoreport, ./e2e/cmd/devsy-ci] + packages: + - ./e2e/framework/runenv/... + - ./e2e/framework/dockerwsl + - ./e2e/framework/ginkgoreport + - ./e2e/cmd/devsy-ci + - ./e2e/tests/ssh + require_tests: [TestProxyCommandExcerptProtectsFailureReport] args: [-race, -count=1, -timeout=5m] - name: e2e-framework @@ -73,6 +79,21 @@ unit: packages: [./e2e/framework/ghfixture] args: [-race, -count=1, -timeout=90s] + - name: windows-detached-task-storage + description: native Windows detached task storage and process ownership + os: [windows] + packages: [./pkg/command, ./pkg/task, ./pkg/workspace] + require_tests: [TestTaskQueriesRejectUncertainRecords, TestGetWaitsForWriterHoldingExclusiveFileHandle] + args: [-race, -count=1, -timeout=10m] + + - name: windows-detached-workspace-lifecycle + description: native Windows detached workers, followers, and task results + os: [windows] + packages: [./cmd/workspace/up, ./cmd/workspace] + run: '^Test(Detached.*|DetachWorkspaceLabel.*|WorkerRecords.*|DuplicateWorkerCannotFailOwnerTask|FinalizeWorker.*|OpenTaskExitsWhenAlreadyCanceled|FollowTask.*|LogTailer.*|ResultEnvelopeFrom.*|EmitTaskTransition|ReportTaskStateJSON.*|TaskErrorMessage|TaskStatusEvent.*)$' + require_tests: [TestWorkerRecordsConfigAndClientErrors, TestDetachedWorkerCommitsBeforeWorkspaceWork, TestFollowTaskReconcilesExitedWorker] + args: [-race, -count=1, -timeout=10m] + - name: windows-transport description: native Windows Docker transport os: [windows] diff --git a/pkg/command/background.go b/pkg/command/background.go index bd40a55b59..a1d7ed9b9c 100644 --- a/pkg/command/background.go +++ b/pkg/command/background.go @@ -14,8 +14,11 @@ import ( type CreateCommand func() (*exec.Cmd, error) type SupervisedStartOptions struct { - Name string - OnStarted func(ProcessRef) error + Name string + OnStarted func(ProcessRef) error + Lease *LaunchLease + AtomicOwnership bool + OnPrepared func(string) error } // StartBackgroundOnce starts a background process, ensuring only one instance @@ -160,18 +163,22 @@ func startSupervisedDetached(cmd *exec.Cmd, opts SupervisedStartOptions, streams if err != nil { return err } - prepareBackgroundTree(cmd) - if err := cmd.Start(); err != nil { + if opts.Lease != nil { + opts.Lease.attach(cmd) + } + release, err := startOwnedBackground(cmd, opts.Name, opts.AtomicOwnership, opts.OnPrepared) + if err != nil { closeFile(streamsF) return fmt.Errorf("start process: %w", err) } + defer release() closeFile(streamsF) pid := cmd.Process.Pid cleanup := func() { _ = abortSupervisedLaunch(pid, opts.Name) _ = cmd.Wait() } - if err := ownProcessTree(pid, opts.Name); err != nil { + if err := opts.ownTree(pid); err != nil { cleanup() return fmt.Errorf("assign process tree ownership for pid %d: %w", pid, err) } @@ -185,7 +192,7 @@ func startSupervisedDetached(cmd *exec.Cmd, opts SupervisedStartOptions, streams cleanup() return fmt.Errorf("publish process reference for pid %d: %w", pid, err) } - if err := resumeBackgroundTree(pid); err != nil { + if err := opts.resumeTree(pid); err != nil { _ = terminateProcessRef(ref) cleanup() return fmt.Errorf("resume process %d: %w", pid, err) @@ -194,6 +201,20 @@ func startSupervisedDetached(cmd *exec.Cmd, opts SupervisedStartOptions, streams return nil } +func (o SupervisedStartOptions) ownTree(pid int) error { + if o.AtomicOwnership { + return nil + } + return ownProcessTree(pid, o.Name) +} + +func (o SupervisedStartOptions) resumeTree(pid int) error { + if o.AtomicOwnership { + return nil + } + return resumeBackgroundTree(pid) +} + func isProcessRunning(pidFile string) (bool, error) { pid, err := os.ReadFile(pidFile) // #nosec G304: not user input if err != nil { diff --git a/pkg/command/job_windows.go b/pkg/command/job_windows.go index 57bdc4156f..6b5f88bbfb 100644 --- a/pkg/command/job_windows.go +++ b/pkg/command/job_windows.go @@ -87,7 +87,7 @@ func canonicalDirectoryPath(path string) (string, error) { if err != nil { if errors.Is(err, windows.ERROR_FILE_NOT_FOUND) || errors.Is(err, windows.ERROR_PATH_NOT_FOUND) { - return normalizedWindowsPath(absPath), nil + return canonicalMissingDirectoryPath(absPath, err) } return "", err } @@ -105,15 +105,16 @@ func canonicalDirectoryPath(path string) (string, error) { return windows.UTF16ToString(buffer[:length]), nil } -func normalizedWindowsPath(path string) string { - path = filepath.Clean(path) - if strings.HasPrefix(path, `\\?\`) { - return path +func canonicalMissingDirectoryPath(absPath string, lookupErr error) (string, error) { + parent := filepath.Dir(absPath) + if parent == absPath { + return "", lookupErr } - if strings.HasPrefix(path, `\\`) { - return `\\?\UNC\` + strings.TrimPrefix(path, `\\`) + canonicalParent, err := canonicalDirectoryPath(parent) + if err != nil { + return "", err } - return `\\?\` + path + return filepath.Join(canonicalParent, filepath.Base(absPath)), nil } func prepareBackgroundTree(cmd *exec.Cmd) { diff --git a/pkg/command/launch_unix.go b/pkg/command/launch_unix.go new file mode 100644 index 0000000000..54918ce14b --- /dev/null +++ b/pkg/command/launch_unix.go @@ -0,0 +1,90 @@ +//go:build linux || darwin || unix + +package command + +import ( + "errors" + "fmt" + "os" + "os/exec" + "strconv" + + "golang.org/x/sys/unix" +) + +const launchLeaseEnv = "DEVSY_INTERNAL_LAUNCH_LEASE_FD" + +// LaunchLease survives launcher death until the worker exits, including the +// interval before its process reference has been published. +type LaunchLease struct{ file *os.File } + +func NewLaunchLease(path string) (*LaunchLease, error) { + f, err := os.OpenFile( + path, + os.O_CREATE|os.O_RDWR, + 0o600, + ) // #nosec G304 -- validated task-store lease path + if err != nil { + return nil, err + } + if err := unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil { + _ = f.Close() + return nil, err + } + return &LaunchLease{file: f}, nil +} + +func (l *LaunchLease) Close() error { return l.file.Close() } + +func (l *LaunchLease) attach(cmd *exec.Cmd) { + fd := 3 + len(cmd.ExtraFiles) + cmd.ExtraFiles = append(cmd.ExtraFiles, l.file) + if cmd.Env == nil { + cmd.Env = os.Environ() + } + cmd.Env = append(cmd.Env, launchLeaseEnv+"="+strconv.Itoa(fd)) +} + +// RetainLaunchLease keeps the inherited claim until process exit, without +// passing it on to commands the worker executes. +func RetainLaunchLease() error { + value := os.Getenv(launchLeaseEnv) + if value == "" { + return errors.New("detached worker launch lease is missing") + } + fd, err := strconv.Atoi(value) + if err != nil || fd < 3 { + return errors.New("invalid detached worker launch lease") + } + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + return err + } + unix.CloseOnExec(fd) + return os.Unsetenv(launchLeaseEnv) +} + +// CleanupUnpublishedLaunch cannot signal an unidentified process. An available +// lifetime lease proves neither the launcher nor its bootstrap child remains. +func CleanupUnpublishedLaunch(path, _ string, _ bool) error { + lease, err := NewLaunchLease(path) + if err != nil { + return fmt.Errorf("detached launch still owns its lifetime lease: %w", err) + } + return lease.Close() +} + +func startOwnedBackground( + cmd *exec.Cmd, + _ string, + _ bool, + prepared func(string) error, +) (func(), error) { + prepareBackgroundTree(cmd) + if prepared != nil { + if err := prepared(""); err != nil { + return nil, err + } + } + return func() {}, cmd.Start() +} diff --git a/pkg/command/launch_unix_test.go b/pkg/command/launch_unix_test.go new file mode 100644 index 0000000000..12c905b23b --- /dev/null +++ b/pkg/command/launch_unix_test.go @@ -0,0 +1,72 @@ +//go:build linux || darwin || unix + +package command + +import ( + "bufio" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +func TestLaunchLeaseSurvivesLauncherCloseUntilChildExit(t *testing.T) { + path := filepath.Join(t.TempDir(), "lease") + lease, err := NewLaunchLease(path) + require.NoError(t, err) + t.Cleanup(func() { _ = lease.Close() }) + child := exec.Command( + os.Args[0], + "-test.run=^TestLaunchLeaseHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + child.Env = append(os.Environ(), "DEVSY_TEST_LAUNCH_LEASE=1") + lease.attach(child) + input, err := child.StdinPipe() + require.NoError(t, err) + output, err := child.StdoutPipe() + require.NoError(t, err) + child.Stderr = os.Stderr + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = input.Close(); _ = child.Process.Kill() }) + ready := make(chan bool, 1) + go func() { scanner := bufio.NewScanner(output); ready <- scanner.Scan() && scanner.Text() == "ready" }() + select { + case ok := <-ready: + require.True(t, ok) + case <-time.After(5 * time.Second): + t.Fatal("child did not retain the launch lease") + } + require.NoError(t, lease.Close()) + require.Error( + t, + CleanupUnpublishedLaunch(path, "", true), + "the unidentified child must retain positive lifetime evidence", + ) + require.NoError(t, input.Close()) + require.NoError(t, child.Wait()) + require.NoError(t, CleanupUnpublishedLaunch(path, "", true)) +} + +func TestLaunchLeaseHelper(t *testing.T) { + if os.Getenv("DEVSY_TEST_LAUNCH_LEASE") == "" { + return + } + fd, err := strconv.Atoi(os.Getenv(launchLeaseEnv)) + require.NoError(t, err) + require.NoError(t, RetainLaunchLease()) + flags, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + require.NoError(t, err) + require.NotZero(t, flags&unix.FD_CLOEXEC) + require.Empty(t, os.Getenv(launchLeaseEnv)) + _, err = fmt.Fprintln(os.Stdout, "ready") + require.NoError(t, err) + _, err = io.Copy(io.Discard, os.Stdin) + require.NoError(t, err) +} diff --git a/pkg/command/launch_windows.go b/pkg/command/launch_windows.go new file mode 100644 index 0000000000..5b55ae0172 --- /dev/null +++ b/pkg/command/launch_windows.go @@ -0,0 +1,272 @@ +//go:build windows + +package command + +import ( + "errors" + "fmt" + "os" + "os/exec" + "slices" + "strings" + "unsafe" + + "golang.org/x/sys/windows" +) + +// Windows uses a named Job Object as the launch lifetime claim. +type LaunchLease struct{} + +func NewLaunchLease(_ string) (*LaunchLease, error) { return &LaunchLease{}, nil } +func (*LaunchLease) Close() error { return nil } +func (*LaunchLease) attach(_ *exec.Cmd) {} +func RetainLaunchLease() error { return nil } + +func CleanupUnpublishedLaunch(_, name string, prepared bool) error { + if !prepared { + return nil + } + _, err := terminateNamedJob(name) + return err +} + +func startOwnedBackground( + cmd *exec.Cmd, + name string, + atomicOwnership bool, + prepared func(string) error, +) (func(), error) { + if !atomicOwnership { + prepareBackgroundTree(cmd) + return func() {}, cmd.Start() + } + return startInWorkerJob(cmd, name, prepared) +} + +// Assigning the job in CreateProcess closes the crash window between creating +// a suspended process and assigning it to its recoverable ownership object. +func startInWorkerJob( + cmd *exec.Cmd, + workerName string, + prepared func(string) error, +) (release func(), err error) { + if err := validateAtomicWorker(cmd); err != nil { + return nil, err + } + job, jobID, err := createLaunchJob(workerName) + if err != nil { + return nil, err + } + closeJob := func() { _ = windows.CloseHandle(job) } + defer func() { + if err != nil { + _, cleanupErr := terminateJobHandle(job, workerName) + err = errors.Join(err, cleanupErr) + closeJob() + } + }() + si, closeStartup, err := workerStartupInfo(cmd, job) + if err != nil { + return nil, err + } + defer closeStartup() + if err := notifyLaunchPrepared(prepared, jobID); err != nil { + return nil, err + } + pi, err := createOwnedWorker(cmd, si) + if err != nil { + return nil, err + } + defer func() { _ = windows.CloseHandle(pi.Thread); _ = windows.CloseHandle(pi.Process) }() + if err := retainWorkerJob(cmd, job, pi); err != nil { + return nil, err + } + return closeJob, nil +} + +func validateAtomicWorker(cmd *exec.Cmd) error { + if cmd.Err != nil { + return cmd.Err + } + if cmd.SysProcAttr != nil || cmd.Process != nil || cmd.Cancel != nil || cmd.WaitDelay != 0 { + return errors.New( + "atomic worker launch requires an unstarted command without process attributes or cancellation", + ) + } + return nil +} + +func createLaunchJob(workerName string) (windows.Handle, string, error) { + jobName, err := jobNameFor(workerName) + if err != nil { + return 0, "", err + } + name, err := windows.UTF16PtrFromString(jobName) + if err != nil { + return 0, "", err + } + job, err := createWorkerJobObject(name) + if err != nil { + return 0, "", err + } + var limits windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION + limits.BasicLimitInformation.LimitFlags = windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE + if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&limits)), uint32(unsafe.Sizeof(limits))); err != nil { + _ = windows.CloseHandle(job) + return 0, "", err + } + return job, jobName, nil +} + +func workerStartupInfo(cmd *exec.Cmd, job windows.Handle) (*windows.StartupInfoEx, func(), error) { + attributes, err := windows.NewProcThreadAttributeList(2) + if err != nil { + return nil, nil, err + } + const procThreadAttributeJobList = 0x0002000d + if err := attributes.Update( + procThreadAttributeJobList, + unsafe.Pointer(&job), + unsafe.Sizeof(job), + ); err != nil { + attributes.Delete() + return nil, nil, fmt.Errorf("configure creation-time worker job: %w", err) + } + handles, closeHandles, err := workerStandardHandles(cmd) + if err != nil { + attributes.Delete() + return nil, nil, err + } + cleanup := func() { closeHandles(); attributes.Delete() } + if err := attributes.Update(windows.PROC_THREAD_ATTRIBUTE_HANDLE_LIST, + unsafe.Pointer(&handles[0]), uintptr(len(handles))*unsafe.Sizeof(handles[0])); err != nil { + cleanup() + return nil, nil, err + } + si := &windows.StartupInfoEx{ + StartupInfo: windows.StartupInfo{ + Cb: uint32(unsafe.Sizeof(windows.StartupInfoEx{})), Flags: windows.STARTF_USESTDHANDLES, + StdInput: handles[0], StdOutput: handles[1], StdErr: handles[2], + }, + ProcThreadAttributeList: attributes.List(), + } + return si, cleanup, nil +} + +func notifyLaunchPrepared(prepared func(string) error, jobID string) error { + if prepared == nil { + return nil + } + return prepared(jobID) +} + +func createOwnedWorker( + cmd *exec.Cmd, + si *windows.StartupInfoEx, +) (*windows.ProcessInformation, error) { + application, err := windows.UTF16PtrFromString(cmd.Path) + if err != nil { + return nil, err + } + args := cmd.Args + if len(args) == 0 { + args = []string{cmd.Path} + } + commandLine, err := windows.UTF16PtrFromString(windows.ComposeCommandLine(args)) + if err != nil { + return nil, err + } + var directory *uint16 + if cmd.Dir != "" { + directory, err = windows.UTF16PtrFromString(cmd.Dir) + if err != nil { + return nil, err + } + } + block, err := workerEnvironment(cmd.Environ()) + if err != nil { + return nil, err + } + pi := new(windows.ProcessInformation) + err = windows.CreateProcess(application, commandLine, nil, nil, true, + windows.CREATE_UNICODE_ENVIRONMENT|windows.EXTENDED_STARTUPINFO_PRESENT, + &block[0], directory, &si.StartupInfo, pi) + if err != nil { + return nil, err + } + return pi, nil +} + +func retainWorkerJob(cmd *exec.Cmd, job windows.Handle, pi *windows.ProcessInformation) error { + // Until the worker holds this handle, launcher death closes the last handle + // and the kernel kills the child. Afterwards recovery can open the named job. + var childJob windows.Handle + if err := windows.DuplicateHandle(windows.CurrentProcess(), job, pi.Process, &childJob, + 0, false, windows.DUPLICATE_SAME_ACCESS); err != nil { + return err + } + var err error + cmd.Process, err = os.FindProcess(int(pi.ProcessId)) + return err +} + +func workerStandardHandles(cmd *exec.Cmd) ([]windows.Handle, func(), error) { + handles := make([]windows.Handle, 0, 3) + closeHandles := func() { + for _, h := range handles { + _ = windows.CloseHandle(h) + } + } + null, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + if err != nil { + return nil, closeHandles, err + } + defer func() { _ = null.Close() }() + for _, stream := range []any{cmd.Stdin, cmd.Stdout, cmd.Stderr} { + file := null + if stream != nil { + var ok bool + file, ok = stream.(*os.File) + if !ok { + closeHandles() + return nil, func() {}, errors.New("atomic worker streams must be files") + } + } + var inherited windows.Handle + if err := windows.DuplicateHandle( + windows.CurrentProcess(), + windows.Handle(file.Fd()), + windows.CurrentProcess(), + &inherited, + 0, + true, + windows.DUPLICATE_SAME_ACCESS, + ); err != nil { + closeHandles() + return nil, func() {}, err + } + handles = append(handles, inherited) + } + return handles, closeHandles, nil +} + +func workerEnvironment(env []string) ([]uint16, error) { + env = slices.Clone(env) + slices.SortFunc( + env, + func(a, b string) int { return strings.Compare(strings.ToUpper(a), strings.ToUpper(b)) }, + ) + var block []uint16 + for _, entry := range env { + encoded, err := windows.UTF16FromString(entry) + if err != nil { + return nil, err + } + block = append(block, encoded...) + } + if len(block) == 0 { + block = append(block, 0) + } + return append(block, 0), nil +} diff --git a/pkg/command/launch_windows_test.go b/pkg/command/launch_windows_test.go new file mode 100644 index 0000000000..a993070413 --- /dev/null +++ b/pkg/command/launch_windows_test.go @@ -0,0 +1,65 @@ +//go:build windows + +package command + +import ( + "os" + "os/exec" + "strconv" + "testing" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +func TestAtomicLaunchRejectsExistingJobWithoutClaimingIt(t *testing.T) { + t.Setenv(config.EnvHome, t.TempDir()) + const name = "devsy-launch-collision-test" + job, _, err := createLaunchJob(name) + require.NoError(t, err) + t.Cleanup(func() { _ = windows.CloseHandle(job) }) + worker := startHelper(t) + process, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(worker.Process.Pid), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = windows.CloseHandle(process) }) + require.NoError(t, windows.AssignProcessToJobObject(job, process)) + cmd := exec.Command( + os.Args[0], + "-test.run=^TestAtomicLaunchRejectsExistingJobWithoutClaimingIt$", + ) // #nosec G204 G702 -- re-exec this test binary + prepared := false + _, err = startInWorkerJob(cmd, name, func(string) error { prepared = true; return nil }) + require.ErrorContains(t, err, "already exists") + require.False(t, prepared) + require.Nil(t, cmd.Process) + require.NoError(t, CleanupUnpublishedLaunch("", name, prepared)) + running, err := IsRunning(strconv.Itoa(worker.Process.Pid)) + require.NoError(t, err) + require.True(t, running, "a failed ownership claim must not terminate the existing job") +} + +func TestLaunchJobPublicationKeepsCreatedIdentity(t *testing.T) { + t.Setenv(config.EnvHome, t.TempDir()) + job, jobID, err := createLaunchJob("devsy-launch-publication-test") + require.NoError(t, err) + t.Cleanup(func() { _ = windows.CloseHandle(job) }) + t.Setenv(config.EnvHome, t.TempDir()) + var published string + require.NoError(t, notifyLaunchPrepared(func(id string) error { + published = id + return nil + }, jobID)) + owned, found, err := openNamedJob(published, published) + require.NoError(t, err) + require.True( + t, + found, + "publication must identify the created job after the home directory changes", + ) + t.Cleanup(func() { _ = windows.CloseHandle(owned) }) +} diff --git a/pkg/command/process.go b/pkg/command/process.go index c58c6f3172..2eb4b0d024 100644 --- a/pkg/command/process.go +++ b/pkg/command/process.go @@ -64,3 +64,12 @@ func KillTreeAfterWorkerExit(pid, treeName, identity string) error { func ProcessTreeIdentity(pid int) (string, error) { return processTreeIdentity(pid) } + +// ProcessExited reports positive evidence that the saved worker has exited. +// Identityless records cannot distinguish a worker from a reused PID. +func ProcessExited(ref ProcessRef) (bool, error) { + if ref.PID <= 0 || ref.Identity == "" || !strongProcessIdentitySupported { + return false, nil + } + return processRefExited(ref) +} diff --git a/pkg/command/process_exit_child_supported_test.go b/pkg/command/process_exit_child_supported_test.go new file mode 100644 index 0000000000..6873ceff87 --- /dev/null +++ b/pkg/command/process_exit_child_supported_test.go @@ -0,0 +1,14 @@ +//go:build linux || darwin || unix + +package command + +import ( + "os/exec" + "syscall" +) + +func processExitTestChild() *exec.Cmd { + child := exec.Command("sleep", "30") + child.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + return child +} diff --git a/pkg/command/process_exit_child_windows_test.go b/pkg/command/process_exit_child_windows_test.go new file mode 100644 index 0000000000..d664de08aa --- /dev/null +++ b/pkg/command/process_exit_child_windows_test.go @@ -0,0 +1,7 @@ +package command + +import "os/exec" + +func processExitTestChild() *exec.Cmd { + return exec.Command("ping", "-n", "30", "127.0.0.1") +} diff --git a/pkg/command/process_exit_test.go b/pkg/command/process_exit_test.go new file mode 100644 index 0000000000..cecbfeef36 --- /dev/null +++ b/pkg/command/process_exit_test.go @@ -0,0 +1,80 @@ +package command + +import ( + "errors" + "path/filepath" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestProcessExitedRecognizesUnreapedChild(t *testing.T) { + child := processExitTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill(); _ = child.Wait() }) + identity, err := ProcessTreeIdentity(child.Process.Pid) + require.NoError(t, err) + ref := ProcessRef{PID: child.Process.Pid, Identity: identity} + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) + require.NoError(t, child.Process.Kill()) + require.Eventually(t, func() bool { + exited, err := ProcessExited(ref) + return err == nil && exited + }, time.Second, time.Millisecond, "exit must be observable before the parent reaps the child") +} + +func TestProcessExitedRequiresSavedWorkerIdentity(t *testing.T) { + child := processExitTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill() }) + identity, err := ProcessTreeIdentity(child.Process.Pid) + require.NoError(t, err) + ref := ProcessRef{PID: child.Process.Pid, Identity: identity} + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) + stale := ref + stale.Identity += "-old" + exited, err = ProcessExited(stale) + require.NoError(t, err) + require.True(t, exited) + // A stale identity describes an exited worker, never authority to signal the current PID. + exited, err = ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + exited, err = ProcessExited(ref) + require.NoError(t, err) + require.True(t, exited) + ref.Identity = "" + exited, err = ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) +} + +func TestSupervisedPublicationFailureCleansUnclaimedWorker(t *testing.T) { + t.Setenv(config.EnvHome, t.TempDir()) + child := processExitTestChild() + var ref ProcessRef + publicationErr := errors.New("publication rejected") + err := startSupervisedDetached(child, SupervisedStartOptions{ + Name: "test-publication-failure", + OnStarted: func(started ProcessRef) error { + ref = started + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited, "a published child waiting to resume is healthy") + return publicationErr + }, + }, filepath.Join(t.TempDir(), "streams")) + require.ErrorIs(t, err, publicationErr) + require.Positive(t, ref.PID) + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.True(t, exited) +} diff --git a/pkg/command/process_supported.go b/pkg/command/process_supported.go index 8ddeced62b..e7c1a246d6 100644 --- a/pkg/command/process_supported.go +++ b/pkg/command/process_supported.go @@ -423,3 +423,21 @@ func prepareBackgroundTree(cmd *exec.Cmd) { } cmd.SysProcAttr.Setsid = true } + +func processRefExited(ref ProcessRef) (bool, error) { + live, err := processIsLive(ref.PID) + if err != nil { + return false, err + } + if !live { + return true, nil + } + current, err := processTreeIdentity(ref.PID) + if err != nil { + return false, err + } + if current == "" || current != ref.Identity { + return true, nil + } + return false, nil +} diff --git a/pkg/command/process_windows.go b/pkg/command/process_windows.go index 0f5ac8c1af..ef2e2ad968 100644 --- a/pkg/command/process_windows.go +++ b/pkg/command/process_windows.go @@ -406,3 +406,11 @@ func truncateOutput(output string) string { } return output } + +func processRefExited(ref ProcessRef) (bool, error) { + running, err := savedWorkerRunning(ref) + if err != nil { + return false, err + } + return !running, nil +} diff --git a/pkg/command/process_windows_test.go b/pkg/command/process_windows_test.go index 29ee3f60eb..807011a9c0 100644 --- a/pkg/command/process_windows_test.go +++ b/pkg/command/process_windows_test.go @@ -735,3 +735,75 @@ func TestKillTerminatesOrphanedGrandchildViaJobObject(t *testing.T) { t.Fatalf("KillTreeAfterWorkerExit after natural tree cleanup: %v", err) } } + +func TestJobNameForMissingNestedHomeUsesCanonicalAncestor(t *testing.T) { + parent := filepath.Join(t.TempDir(), "parent") + home := filepath.Join(parent, "home") + if err := os.MkdirAll(home, 0o700); err != nil { + t.Fatal(err) + } + t.Setenv(config.EnvHome, home) + before, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if err := os.Remove(home); err != nil { + t.Fatal(err) + } + if err := os.Remove(parent); err != nil { + t.Fatal(err) + } + after, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if before != after { + t.Fatalf("deleted nested home changed job identity: %q != %q", before, after) + } +} + +func TestJobNameForDeletedHomePreservesShortParentAlias(t *testing.T) { + parent := filepath.Join(t.TempDir(), "long parent directory for short alias") + if err := os.Mkdir(parent, 0o700); err != nil { + t.Fatal(err) + } + parentPtr, err := windows.UTF16PtrFromString(parent) + if err != nil { + t.Fatal(err) + } + buffer := make([]uint16, 32768) + length, err := windows.GetShortPathName(parentPtr, &buffer[0], uint32(len(buffer))) + if err != nil { + t.Fatal(err) + } + if length >= uint32(len(buffer)) { + t.Fatal("short parent path exceeds buffer") + } + shortParent := windows.UTF16ToString(buffer[:length]) + if strings.EqualFold(parent, shortParent) { + t.Skip("filesystem does not provide an 8.3 alias") + } + home := filepath.Join(shortParent, "home") + if err := os.Mkdir(home, 0o700); err != nil { + t.Fatal(err) + } + t.Setenv(config.EnvHome, home) + before, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if err := os.Remove(home); err != nil { + t.Fatal(err) + } + after, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if before != after { + t.Fatalf( + "deleted home with short parent alias changed job identity: %q != %q", + before, + after, + ) + } +} diff --git a/pkg/task/launch.go b/pkg/task/launch.go new file mode 100644 index 0000000000..cb7d6892e4 --- /dev/null +++ b/pkg/task/launch.go @@ -0,0 +1,305 @@ +package task + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/devsy-org/devsy/pkg/clierr" + "github.com/devsy-org/devsy/pkg/command" +) + +func (s *Store) launchLeasePath(id string) (string, error) { + path, err := s.path(id) + if err != nil { + return "", err + } + return path + ".launch.lease", nil +} + +func (t *Task) LaunchLease() *command.LaunchLease { return t.launchLease } + +func (t *Task) PrepareProcessOwnership(treeID string) error { + accepted := false + err := t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.LaunchOwned { + return + } + s.LaunchOwned = true + s.LaunchTreeID = treeID + accepted = true + }) + if err != nil { + return err + } + if !accepted { + return ErrCanceled + } + return nil +} + +func (t *Task) FailLaunch(cause error) error { + failErr := t.recordFailure(cause, true) + releaseErr := releaseLauncherLock(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, cleanupErr := t.store.reconcileLaunch(ctx, t.id, true) + return errors.Join(failErr, releaseErr, cleanupErr) +} + +// PublishProcess retains launcher ownership until the worker acknowledges the +// matching process reference and commit authorizes workspace work. +func (t *Task) PublishProcess(ref command.ProcessRef) error { + if ref.PID <= 0 || ref.Identity == "" { + return errors.New("worker launch requires a strong process reference") + } + accepted := false + err := t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.Process != nil { + return + } + s.Process = &ref + s.PID = ref.PID + s.ProcessTreeIdentity = ref.Identity + accepted = true + }) + if err != nil { + return err + } + if !accepted { + return ErrCanceled + } + return nil +} + +func (t *Task) WorkerLaunchReady(pid int) error { + identity, err := command.ProcessTreeIdentity(pid) + if err != nil { + return err + } + accepted := false + err = t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.PID != pid || identity == "" || + s.ProcessTreeIdentity != identity { + return + } + s.WorkerReady = true + accepted = true + }) + if err != nil { + return err + } + if !accepted { + return ErrCanceled + } + return nil +} + +func (t *Task) CommitLaunch(ctx context.Context) error { + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { + if err := ctx.Err(); err != nil { + return err + } + committed, err := t.commitLaunchStep() + if err != nil { + return err + } + if committed { + return releaseLauncherLock(t) + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + } + } +} + +func (t *Task) LauncherAlive() (bool, error) { + lock, available, err := t.store.tryLauncherLock(t.id) + if err != nil { + return false, err + } + if available { + return false, lock.Unlock() + } + return true, nil +} + +func (s *Store) reconcilePendingLaunch(ctx context.Context, id string) (*State, error) { + return s.reconcileLaunch(ctx, id, false) +} + +func (s *Store) reconcileLaunch(ctx context.Context, id string, aborting bool) (*State, error) { + lock, available, err := s.tryLauncherLock(id) + if err != nil { + return nil, err + } + if !available { + return s.Get(id) + } + defer func() { _ = lock.Unlock() }() + state, err := s.Get(id) + if err != nil || (!state.LaunchPending && !aborting) { + return state, err + } + if state.ProcessCleanupComplete { + return state, nil + } + if err := s.cleanupPendingLaunch(ctx, state); err != nil { + return state, err + } + return s.finishAbandonedLaunch(id, aborting) +} + +func (s *Store) cleanupPendingLaunch(ctx context.Context, state *State) error { + if err := ctx.Err(); err != nil { + return err + } + if state.LaunchProtocol != 1 { + return errors.New("cannot verify cleanup of a legacy abandoned launch") + } + if ref, ok := state.ProcessReference(); ok { + if err := s.processController.Terminate(ref); err != nil { + return err + } + if err := s.processController.CleanupAfterExit(ref); err != nil { + return err + } + } + path, err := s.launchLeasePath(state.ID) + if err != nil { + return err + } + if err := command.CleanupUnpublishedLaunch( + path, + state.LaunchTreeID, + state.LaunchOwned, + ); err != nil { + return fmt.Errorf("clean up abandoned launch %s: %w", state.ID, err) + } + return nil +} + +func (s *Store) finishAbandonedLaunch(id string, aborting bool) (*State, error) { + err := s.update(id, func(current *State) { + if !current.LaunchPending && !aborting { + return + } + current.LaunchPending = false + current.ProcessCleanupComplete = true + if current.Status.Terminal() { + return + } + if current.CancelRequested { + markCanceled(current) + return + } + failure := clierr.Classify(ErrAbandoned) + current.Status = StatusFailed + current.Error = failure.Message + current.ErrorCode = string(failure.Code) + current.ErrorHint = failure.Hint + }) + if err != nil { + return nil, err + } + return s.Get(id) +} + +func (s *State) acceptsLaunch() bool { + return s.LaunchProtocol == 1 && s.LaunchPending && !s.Status.Terminal() && !s.CancelRequested +} + +func (s *State) hasPendingLaunch() bool { return s != nil && s.LaunchPending } + +func (s *State) markLaunchCleanupPending() { + if s.LaunchProtocol == 1 && !s.ProcessCleanupComplete { + s.LaunchPending = true + } +} + +func (t *Task) commitLaunchStep() (bool, error) { + committed, rejected := false, false + err := t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.Process == nil { + rejected = true + return + } + if !s.WorkerReady { + return + } + s.LaunchPending = false + committed = true + }) + if err != nil { + return false, err + } + if rejected { + return false, ErrCanceled + } + return committed, nil +} + +func (t *Task) claimLaunchOwnership() error { + launcherLock, available, err := t.store.tryLauncherLock(t.id) + if err != nil { + return fmt.Errorf("lock task %s launcher: %w", t.id, err) + } + if !available { + return fmt.Errorf("task %s already has a launcher", t.id) + } + leasePath, err := t.store.launchLeasePath(t.id) + if err != nil { + _ = launcherLock.Unlock() + return err + } + lease, err := command.NewLaunchLease(leasePath) + if err != nil { + _ = launcherLock.Unlock() + return err + } + t.launcherLock = launcherLock + t.launchLease = lease + return nil +} + +func (s *State) checkDeletion(force bool) error { + if s.LaunchPending || s.NeedsExitedWorkerCleanup() || s.NeedsCanceledWorkerCleanup() { + return fmt.Errorf("task %s still needs process tree cleanup", s.ID) + } + if !force && !s.Status.Terminal() { + return fmt.Errorf( + "task %s is still %s; cancel it first or delete with force", + s.ID, + s.Status, + ) + } + return nil +} + +func (s *Store) stateAfterLaunchCleanup(id string) (*State, error) { + state, err := s.Get(id) + if err != nil || !state.LaunchPending { + return state, err + } + state, err = s.reconcilePendingLaunch(context.Background(), id) + if err != nil { + return nil, err + } + if state.LaunchPending { + return nil, fmt.Errorf("task %s launch is still owned", id) + } + return state, nil +} + +func (s *Store) pendingLaunchState(state *State) *State { + current, err := s.reconcilePendingLaunch(context.Background(), state.ID) + if err != nil { + return state + } + return current +} diff --git a/pkg/task/launch_test.go b/pkg/task/launch_test.go new file mode 100644 index 0000000000..026f4d253f --- /dev/null +++ b/pkg/task/launch_test.go @@ -0,0 +1,324 @@ +package task + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestReconcileLaunchAbandonedBeforeSpawn(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + require.NoError(t, tk.BeginLaunch()) + require.NoError(t, releaseLauncherLock(tk)) + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + require.False(t, state.LaunchPending) + require.True(t, state.ProcessCleanupComplete) +} + +func TestLaunchCommitRequiresReadinessAndPreservesCancellation(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, releaseLauncherLock(tk)) }) + identity, err := command.ProcessTreeIdentity(os.Getpid()) + require.NoError(t, err) + ref := command.ProcessRef{PID: os.Getpid(), Identity: identity} + require.NoError(t, tk.PublishProcess(ref)) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + require.ErrorIs(t, tk.CommitLaunch(ctx), context.Canceled) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.ErrorIs(t, tk.WorkerLaunchReady(os.Getpid()+1), ErrCanceled) + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + _, err = tk.requestCancel() + require.NoError(t, err) + require.ErrorIs(t, tk.CommitLaunch(context.Background()), ErrCanceled) + state, err = store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.True(t, state.CancelRequested) + require.NoError(t, releaseLauncherLock(tk)) + store.processController = fakeProcessController{} + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.True(t, state.Canceled()) + require.True(t, state.ProcessCleanupComplete) +} + +func TestLegacyPendingLaunchCannotClaimCleanup(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + require.NoError(t, store.update(tk.ID(), func(s *State) { s.LaunchPending = true })) + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.ErrorContains(t, err, "legacy abandoned launch") + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) + require.Error(t, tk.Cancel()) + state, err = store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) +} + +func TestFailedLaunchAfterCommitStillCleansTheWorkerTree(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + identity, err := command.ProcessTreeIdentity(os.Getpid()) + require.NoError(t, err) + require.NoError(t, tk.PublishProcess(command.ProcessRef{PID: os.Getpid(), Identity: identity})) + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + require.NoError(t, tk.CommitLaunch(context.Background())) + terminationCalls := 0 + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { terminationCalls++; return nil }, + } + failure := errors.New("launch handoff could not release ownership") + require.NoError(t, tk.FailLaunch(failure)) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, 1, terminationCalls) + require.True(t, state.ProcessCleanupComplete) + require.False(t, state.LaunchPending) + require.Equal(t, StatusFailed, state.Status) + require.Contains(t, state.Error, failure.Error()) + require.NoError(t, tk.FailLaunch(failure)) + require.Equal( + t, + 1, + terminationCalls, + "verified cleanup must not signal the saved reference again", + ) +} + +func TestCommittedLaunchCleanupFailureCanBeRetried(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + identity, err := command.ProcessTreeIdentity(os.Getpid()) + require.NoError(t, err) + require.NoError(t, tk.PublishProcess(command.ProcessRef{PID: os.Getpid(), Identity: identity})) + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + require.NoError(t, tk.CommitLaunch(context.Background())) + cleanupFailure := errors.New("process cleanup unavailable") + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { return cleanupFailure }, + } + require.ErrorIs(t, tk.FailLaunch(errors.New("handoff failed")), cleanupFailure) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) + require.Error(t, store.Delete(tk.ID(), true)) + store.processController = fakeProcessController{} + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.False(t, state.LaunchPending) + require.True(t, state.ProcessCleanupComplete) + require.Contains(t, state.Error, "handoff failed") +} + +func TestAbandonedLaunchCleanupFailureRemainsRetryable(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + require.NoError(t, tk.PublishProcess(command.ProcessRef{PID: 123, Identity: "fixture"})) + require.NoError(t, releaseLauncherLock(tk)) + cleanupFailure := errors.New("cannot verify cleanup") + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { return cleanupFailure }, + } + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.ErrorIs(t, err, cleanupFailure) + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) + require.False(t, state.Status.Terminal()) + require.Error(t, store.Delete(tk.ID(), true)) + store.processController = fakeProcessController{} + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.False(t, state.LaunchPending) + require.True(t, state.ProcessCleanupComplete) + require.Equal(t, StatusFailed, state.Status) +} + +func TestLauncherCrashCannotAuthorizeWorkspaceWork(t *testing.T) { + for _, stage := range []string{"before-spawn", "before-publication", "after-publication", "after-ready"} { + t.Run(stage, func(t *testing.T) { + dir := t.TempDir() + launcher := exec.Command( + os.Args[0], + "-test.run=^TestLaunchCrashHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + launcher.Env = append( + os.Environ(), + "DEVSY_LAUNCH_TEST=launcher", + "DEVSY_LAUNCH_STAGE="+stage, + config.EnvHome+"="+dir, + "DEVSY_LAUNCH_STORE="+filepath.Join(dir, "tasks"), + ) + stdout, err := launcher.StdoutPipe() + require.NoError(t, err) + stdin, err := launcher.StdinPipe() + require.NoError(t, err) + launcher.Stderr = os.Stderr + require.NoError(t, launcher.Start()) + t.Cleanup(func() { _ = stdin.Close(); _ = launcher.Process.Kill() }) + line := make(chan string, 1) + go func() { + scanner := bufio.NewScanner(stdout) + if scanner.Scan() { + line <- scanner.Text() + } else { + line <- "" + } + }() + var id string + select { + case id = <-line: + require.Len(t, id, 12) + case <-time.After(15 * time.Second): + t.Fatal("launcher did not reach crash barrier") + } + store, err := NewStoreAt(filepath.Join(dir, "tasks")) + require.NoError(t, err) + state, err := store.ReconcileTask(context.Background(), id) + require.NoError(t, err) + require.True(t, state.LaunchPending, "a live launcher still owns the handoff") + require.NoError(t, launcher.Process.Kill()) + require.Error(t, launcher.Wait()) + require.Eventually(t, func() bool { + state, err = store.ReconcileTask(context.Background(), id) + return err == nil && !state.LaunchPending && state.ProcessCleanupComplete + }, 15*time.Second, 20*time.Millisecond, "abandoned launch must reconcile with proven cleanup") + require.Equal(t, StatusFailed, state.Status) + _, err = os.Stat(filepath.Join(dir, "workspace-work-authorized")) + require.ErrorIs(t, err, os.ErrNotExist) + }) + } +} + +func TestLaunchCrashHelper(t *testing.T) { + role := os.Getenv("DEVSY_LAUNCH_TEST") + if role == "" { + return + } + config.ResetPathManager() + store, err := NewStoreAt(os.Getenv("DEVSY_LAUNCH_STORE")) + require.NoError(t, err) + if role == "worker" { + runLaunchCrashWorker(t, store) + return + } + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + barrier := func() { + _, err := fmt.Fprintln(os.Stdout, tk.ID()) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, os.Stdin) + } + stage := os.Getenv("DEVSY_LAUNCH_STAGE") + if stage == "before-spawn" { + barrier() + return + } + err = command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: WorkerProcessName( + tk.ID(), + ), + Lease: tk.LaunchLease(), + AtomicOwnership: true, + OnPrepared: tk.PrepareProcessOwnership, + OnStarted: func(ref command.ProcessRef) error { + if stage != "before-publication" { + require.NoError(t, tk.PublishProcess(ref)) + } + if stage == "after-ready" { + require.Eventually( + t, + func() bool { state, err := store.Get(tk.ID()); return err == nil && state.WorkerReady }, + 5*time.Second, + 20*time.Millisecond, + ) + } + barrier() + return fmt.Errorf("test barrier released without commit") + }, + }, func() (*exec.Cmd, error) { + worker := exec.Command( + os.Args[0], + "-test.run=^TestLaunchCrashHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + worker.Env = append(os.Environ(), "DEVSY_LAUNCH_TEST=worker", "DEVSY_LAUNCH_ID="+tk.ID()) + return worker, nil + }) + require.Error(t, err) +} + +func runLaunchCrashWorker(t *testing.T, store *Store) { + tk := store.Open(os.Getenv("DEVSY_LAUNCH_ID")) + require.NoError(t, command.RetainLaunchLease()) + require.NoError(t, tk.HoldWorkerLock()) + defer func() { require.NoError(t, tk.ReleaseWorkerLock()) }() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + deadline := time.NewTimer(10 * time.Second) + defer deadline.Stop() + for { + state, err := store.Get(tk.ID()) + require.NoError(t, err) + if !advanceCrashWorker(t, tk, state) { + return + } + select { + case <-ticker.C: + case <-deadline.C: + t.Fatal("worker bootstrap did not resolve") + } + } +} + +func advanceCrashWorker(t *testing.T, tk *Task, state *State) bool { + if state.Status.Terminal() || state.CancelRequested { + return false + } + if !state.LaunchPending { + require.NoError( + t, + // #nosec G703 -- the parent test supplies its private TempDir as DEVSY_HOME. + os.WriteFile( + filepath.Join(os.Getenv(config.EnvHome), "workspace-work-authorized"), + nil, + 0o600, + ), + ) + return false + } + alive, err := tk.LauncherAlive() + require.NoError(t, err) + if !alive { + return false + } + if state.PID != 0 { + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + } + return true +} diff --git a/pkg/task/legacy_launch_test.go b/pkg/task/legacy_launch_test.go new file mode 100644 index 0000000000..780d2be6dc --- /dev/null +++ b/pkg/task/legacy_launch_test.go @@ -0,0 +1,102 @@ +package task + +import ( + "context" + "os/exec" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/stretchr/testify/require" +) + +func TestLegacyPendingLaunchDoesNotTrustPreviousWorkerExit(t *testing.T) { + for _, scenario := range []struct { + name string + cancelRequested bool + terminal bool + }{ + {name: "pending"}, + {name: "cancel requested", cancelRequested: true}, + {name: "already failed", terminal: true}, + } { + t.Run(scenario.name, func(t *testing.T) { + store, tk, unpublished := ambiguousLegacyLaunch(t) + require.NoError(t, store.update(tk.ID(), func(s *State) { + s.CancelRequested = scenario.cancelRequested + if scenario.terminal { + s.Status = StatusFailed + s.Error = "original worker failure" + s.ErrorCode = "original_code" + } + })) + before, err := store.Get(tk.ID()) + require.NoError(t, err) + terminateCalls, cleanupCalls := 0, 0 + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { terminateCalls++; return nil }, + cleanup: func(command.ProcessRef) error { cleanupCalls++; return nil }, + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + state, err := store.ReconcileTask(ctx, tk.ID()) + require.ErrorContains(t, err, "legacy abandoned launch") + require.Equal(t, before.CancelRequested, state.CancelRequested) + requireLegacyLaunchUnverified(t, before, state) + require.ErrorContains(t, tk.CancelContext(ctx), "legacy abandoned launch") + state, err = store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, !scenario.terminal || before.CancelRequested, state.CancelRequested) + requireLegacyLaunchUnverified(t, before, state) + require.Zero(t, terminateCalls) + require.Zero(t, cleanupCalls) + exited, err := command.ProcessExited(unpublished) + require.NoError(t, err) + require.False(t, exited, "the unpublished attempt must remain alive") + }) + } +} + +func ambiguousLegacyLaunch(t *testing.T) (*Store, *Task, command.ProcessRef) { + t.Helper() + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + previous := legacyLaunchChild(t) + require.NoError(t, tk.SetPID(previous.Process.Pid)) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + previousRef, ok := state.ProcessReference() + require.True(t, ok) + require.NotEmpty(t, previousRef.Identity) + require.NoError(t, previous.Process.Kill()) + require.Error(t, previous.Wait()) + exited, err := command.ProcessExited(previousRef) + require.NoError(t, err) + require.True(t, exited) + unpublished := legacyLaunchChild(t) + identity, err := command.ProcessTreeIdentity(unpublished.Process.Pid) + require.NoError(t, err) + require.NotEmpty(t, identity) + // Legacy BeginLaunch could retain an earlier attempt's reference; the current + // protocol rejects reuse, so this fixture restores that ambiguous stored state. + require.NoError(t, store.update(tk.ID(), func(s *State) { s.LaunchPending = true })) + return store, tk, command.ProcessRef{PID: unpublished.Process.Pid, Identity: identity} +} + +func legacyLaunchChild(t *testing.T) *exec.Cmd { + t.Helper() + child := startupTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill(); _ = child.Wait() }) + return child +} + +func requireLegacyLaunchUnverified(t *testing.T, before, after *State) { + t.Helper() + require.True(t, after.LaunchPending) + require.False(t, after.ProcessCleanupComplete) + require.Equal(t, before.Status, after.Status) + require.Equal(t, before.Error, after.Error) + require.Equal(t, before.ErrorCode, after.ErrorCode) +} diff --git a/pkg/task/read_unix.go b/pkg/task/read_unix.go new file mode 100644 index 0000000000..4f887790dd --- /dev/null +++ b/pkg/task/read_unix.go @@ -0,0 +1,9 @@ +//go:build !windows + +package task + +import "os" + +func openTaskFile(path string) (*os.File, error) { + return os.Open(path) // #nosec G304 G703 -- path validated by Store.path +} diff --git a/pkg/task/read_windows.go b/pkg/task/read_windows.go new file mode 100644 index 0000000000..4eefeecdb2 --- /dev/null +++ b/pkg/task/read_windows.go @@ -0,0 +1,22 @@ +package task + +import ( + "os" + + "golang.org/x/sys/windows" +) + +func openTaskFile(path string) (*os.File, error) { + name, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, err + } + // Readers must allow the atomic state replacement while their handle is open. + handle, err := windows.CreateFile(name, windows.GENERIC_READ, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + return nil, err + } + return os.NewFile(uintptr(handle), path), nil +} diff --git a/pkg/task/read_windows_test.go b/pkg/task/read_windows_test.go new file mode 100644 index 0000000000..aedbfe1c10 --- /dev/null +++ b/pkg/task/read_windows_test.go @@ -0,0 +1,43 @@ +package task + +import ( + "io" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +func TestGetWaitsForWriterHoldingExclusiveFileHandle(t *testing.T) { + assertTaskReadWaitsForWriter(t, func(path string) func() { + name, err := windows.UTF16PtrFromString(path) + require.NoError(t, err) + handle, err := windows.CreateFile(name, windows.GENERIC_READ, 0, + nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + require.NoError(t, err) + return func() { require.NoError(t, windows.CloseHandle(handle)) } + }) +} + +func TestTaskReaderAllowsAtomicStateReplacement(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + path, err := store.path(tk.ID()) + require.NoError(t, err) + reader, err := openTaskFile(path) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, reader.Close()) }) + before, err := io.ReadAll(reader) + require.NoError(t, err) + + require.NoError(t, tk.Fail(ErrAbandoned)) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + _, err = reader.Seek(0, io.SeekStart) + require.NoError(t, err) + after, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, before, after) +} diff --git a/pkg/task/startup_child_supported_test.go b/pkg/task/startup_child_supported_test.go new file mode 100644 index 0000000000..a922ba186d --- /dev/null +++ b/pkg/task/startup_child_supported_test.go @@ -0,0 +1,14 @@ +//go:build linux || darwin || unix + +package task + +import ( + "os/exec" + "syscall" +) + +func startupTestChild() *exec.Cmd { + child := exec.Command("sleep", "30") + child.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + return child +} diff --git a/pkg/task/startup_child_windows_test.go b/pkg/task/startup_child_windows_test.go new file mode 100644 index 0000000000..c9637a08ec --- /dev/null +++ b/pkg/task/startup_child_windows_test.go @@ -0,0 +1,7 @@ +package task + +import "os/exec" + +func startupTestChild() *exec.Cmd { + return exec.Command("ping", "-n", "30", "127.0.0.1") +} diff --git a/pkg/task/startup_test.go b/pkg/task/startup_test.go new file mode 100644 index 0000000000..7bf7f8ccdd --- /dev/null +++ b/pkg/task/startup_test.go @@ -0,0 +1,118 @@ +package task + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/stretchr/testify/require" +) + +func TestReconcileWorkerBeforeClaim(t *testing.T) { + for _, claimed := range []bool{false, true} { + t.Run(map[bool]string{false: "unclaimed", true: "claimed"}[claimed], func(t *testing.T) { + store := newTestStore(t) + store.processController = fakeProcessController{} + tk := createTaskWith(t, store, CreateOptions{Command: "up"}) + child := startupTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill() }) + require.NoError(t, tk.SetPID(child.Process.Pid)) + if claimed { + require.NoError(t, tk.HoldWorkerLock()) + } + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.False(t, state.Status.Terminal(), "healthy startup must not be abandoned") + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + if claimed { + require.NoError(t, tk.ReleaseWorkerLock()) + } + var wg sync.WaitGroup + results := make(chan error, 4) + for range 4 { + wg.Go( + func() { _, err := store.ReconcileTask(context.Background(), tk.ID()); results <- err }, + ) + } + wg.Wait() + close(results) + for err := range results { + require.NoError(t, err) + } + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + require.True(t, state.ProcessCleanupComplete) + }) + } +} + +func TestReconcileUnclaimedWorkerCleanupRetry(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{Command: "up"}) + child := startupTestChild() + require.NoError(t, child.Start()) + require.NoError(t, tk.SetPID(child.Process.Pid)) + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + calls := 0 + store.processController = fakeProcessController{cleanup: func(command.ProcessRef) error { + calls++ + if calls == 1 { + return errors.New("cleanup unavailable") + } + return nil + }} + _, err := store.ReconcileTask(context.Background(), tk.ID()) + require.Error(t, err) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.False(t, state.Status.Terminal()) + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + require.True(t, state.ProcessCleanupComplete) +} + +func TestExitConfirmationNeverCreatesWorkerClaim(t *testing.T) { + path := filepath.Join(t.TempDir(), "worker.lock") + calls := 0 + lock, ok := claimConfirmedWorkerExit(path+".reconcile", func() bool { + calls++ + return calls == 1 + }) + require.False(t, ok) + require.Nil(t, lock) + _, err := os.Stat(path) + require.ErrorIs(t, err, os.ErrNotExist) + lock, ok = claimConfirmedWorkerExit(path+".reconcile", func() bool { return false }) + require.False(t, ok) + require.Nil(t, lock) +} + +func TestUnclaimedWorkerExitPreservesCancellation(t *testing.T) { + store := newTestStore(t) + store.processController = fakeProcessController{} + tk := createTaskWith(t, store, CreateOptions{Command: "up"}) + child := startupTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill() }) + require.NoError(t, tk.SetPID(child.Process.Pid)) + _, err := tk.requestCancel() + require.NoError(t, err) + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.True(t, state.Canceled()) + require.True(t, state.ProcessCleanupComplete) + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.True(t, state.Canceled()) +} diff --git a/pkg/task/store.go b/pkg/task/store.go index 0c44a41d09..e48151a84d 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -3,7 +3,9 @@ package task import ( "context" "encoding/json" + "errors" "fmt" + "io" "os" "path/filepath" "sort" @@ -13,6 +15,7 @@ import ( "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/random" "github.com/gofrs/flock" + "tailscale.com/atomicfile" ) // lockTimeout bounds how long update waits to acquire a task's file lock. @@ -24,6 +27,8 @@ type Store struct { processController command.ProcessController afterClaimHook func() afterCancelLockClaimedHook func() + afterStateLockBlockedHook func() + stateLockTimeout time.Duration } func NewStore() (*Store, error) { @@ -45,20 +50,12 @@ func NewStoreAt(dir string) (*Store, error) { } func (s *Store) Create(opts CreateOptions) (*Task, error) { - id := random.String(12) - now := time.Now() - state := &State{ - ID: id, - Command: opts.Command, - WorkspaceID: opts.WorkspaceID, - Status: StatusPending, - StartedAt: now, - UpdatedAt: now, - } - if err := s.write(state); err != nil { - return nil, err - } - return &Task{store: s, id: id}, nil + return s.create(opts, false) +} + +// CreateLaunch acquires launch ownership before the task becomes visible. +func (s *Store) CreateLaunch(opts CreateOptions) (*Task, error) { + return s.create(opts, true) } // Open returns a handle without reading the task's state. @@ -84,20 +81,12 @@ func (s *Store) Delete(id string, force bool) error { // Locked across the check and the removal so a concurrent update can't // commit its atomic rename after the check and recreate the task. return s.withLock(id, func() error { - state, err := s.Get(id) + state, err := s.readState(id) if err != nil { return err } - if state.NeedsExitedWorkerCleanup() || state.NeedsCanceledWorkerCleanup() { - return fmt.Errorf("task %s still needs process tree cleanup", id) - } - if !force { - if !state.Status.Terminal() { - return fmt.Errorf( - "task %s is still %s; cancel it first or delete with force", - id, state.Status, - ) - } + if err := state.checkDeletion(force); err != nil { + return err } if err := os.Remove(path); err != nil { return fmt.Errorf("delete task %s: %w", id, err) @@ -107,19 +96,13 @@ func (s *Store) Delete(id string, force bool) error { } func (s *Store) Get(id string) (*State, error) { - path, err := s.path(id) - if err != nil { - return nil, err - } - data, err := os.ReadFile(path) // #nosec G304 -- path validated by s.path - if err != nil { - return nil, fmt.Errorf("read task %s: %w", id, err) - } - state := &State{} - if err := json.Unmarshal(data, state); err != nil { - return nil, fmt.Errorf("parse task %s: %w", id, err) - } - return state, nil + var state *State + err := s.withLock(id, func() error { + var err error + state, err = s.readState(id) + return err + }) + return state, err } // Abandoned reports whether a non-terminal task's worker is gone. @@ -145,6 +128,9 @@ func (s *Store) ReconcileTask(ctx context.Context, id string) (*State, error) { if err != nil { return nil, err } + if current.LaunchPending { + return s.reconcilePendingLaunch(ctx, id) + } if !current.NeedsReconciliation() { return current, nil } @@ -163,6 +149,9 @@ func (s *Store) ReconcileTask(ctx context.Context, id string) (*State, error) { // requested cancellation completes cleanup here; an abandoned task's tree is // cleaned up by the next workspace stop or delete. func (s *Store) ReconcileState(state *State) *State { + if state.hasPendingLaunch() { + return s.pendingLaunchState(state) + } if !state.NeedsReconciliation() { return state } @@ -192,7 +181,7 @@ func (s *Store) ReconcileState(state *State) *State { // Claiming the worker lock proves no worker is running, so a saved reference // that still points at live processes belongs to a crashed worker's tree. func (s *Store) CleanupExitedWorkerTree(id string) error { - state, err := s.Get(id) + state, err := s.stateAfterLaunchCleanup(id) if err != nil { return err } @@ -254,11 +243,22 @@ func (s *Store) List() ([]*State, error) { continue } id := entry.Name()[:len(entry.Name())-len(".json")] - state, err := s.Get(id) + var state *State + err := s.withLock(id, func() error { + var readErr error + state, readErr = s.readState(id) + // Only an absent state read under the lock confirms concurrent deletion. + if errors.Is(readErr, os.ErrNotExist) { + return nil + } + return readErr + }) if err != nil { - continue + return nil, fmt.Errorf("list task %s: %w", id, err) + } + if state != nil { + states = append(states, state) } - states = append(states, state) } sort.Slice(states, func(i, j int) bool { @@ -328,6 +328,29 @@ func (s *Store) WaitForWorkerObservation( } } +// The caller must hold the state lock: Windows replacement can briefly make +// the path unavailable, and lock-owning callbacks cannot acquire it again. +func (s *Store) readState(id string) (*State, error) { + path, err := s.path(id) + if err != nil { + return nil, err + } + file, err := openTaskFile(path) + if err != nil { + return nil, fmt.Errorf("read task %s: %w", id, err) + } + defer func() { _ = file.Close() }() + data, err := io.ReadAll(file) + if err != nil { + return nil, fmt.Errorf("read task %s: %w", id, err) + } + state := &State{} + if err := json.Unmarshal(data, state); err != nil { + return nil, fmt.Errorf("parse task %s: %w", id, err) + } + return state, nil +} + func (s *Store) markProcessCleanupComplete(id string) error { return s.update(id, func(state *State) { state.ProcessCleanupComplete = true @@ -515,26 +538,61 @@ func (s *Store) tryLock( return lock, true, nil } -// claimDeadWorkerLock acquires the task's worker lock, which only succeeds when -// no worker holds it. +// claimDeadWorkerLock claims reconciliation ownership after a worker releases +// its lock or its saved identity proves it exited before claiming one. func (s *Store) claimDeadWorkerLock(id string) (*flock.Flock, bool) { path, err := s.workerLockPath(id) if err != nil { return nil, false } - // No lock file means the worker never got far enough to claim one. - if _, statErr := os.Stat(path); statErr != nil { + _, statErr := os.Stat(path) + if errors.Is(statErr, os.ErrNotExist) { + // Reconciliation must never manufacture evidence that a worker claimed its lock. + return claimConfirmedWorkerExit( + path+".reconcile", + func() bool { return s.workerExitConfirmed(id) }, + ) + } + if statErr != nil { return nil, false } + lock := flock.New(path) + locked, err := lock.TryLock() + if err != nil || !locked { + return nil, false + } + return lock, true +} +func claimConfirmedWorkerExit(path string, confirmed func() bool) (*flock.Flock, bool) { + if !confirmed() { + return nil, false + } lock := flock.New(path) locked, err := lock.TryLock() if err != nil || !locked { return nil, false } + if !confirmed() { + _ = lock.Unlock() + return nil, false + } return lock, true } +func (s *Store) workerExitConfirmed(id string) bool { + state, err := s.Get(id) + if err != nil { + return false + } + ref, ok := state.ProcessReference() + if !ok { + return false + } + exited, err := command.ProcessExited(ref) + return err == nil && exited +} + // workerLockPath is distinct from the read-modify-write lock in withLock: this // one is held for the worker's entire lifetime, so sharing it would block // every state update. @@ -556,7 +614,7 @@ func (s *Store) launcherLockPath(id string) (string, error) { func (s *Store) update(id string, mutate func(*State)) error { return s.withLock(id, func() error { - state, err := s.Get(id) + state, err := s.readState(id) if err != nil { return err } @@ -576,9 +634,24 @@ func (s *Store) withLock(id string, fn func() error) error { } lock := flock.New(path + ".lock") - ctx, cancel := context.WithTimeout(context.Background(), lockTimeout) + locked, err := lock.TryLock() + if err != nil { + return fmt.Errorf("lock task %s: %w", id, err) + } + if locked { + defer func() { _ = lock.Unlock() }() + return fn() + } + if s.afterStateLockBlockedHook != nil { + s.afterStateLockBlockedHook() + } + timeout := s.stateLockTimeout + if timeout == 0 { + timeout = lockTimeout + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - locked, err := lock.TryLockContext(ctx, 20*time.Millisecond) + locked, err = lock.TryLockContext(ctx, 20*time.Millisecond) if err != nil { return fmt.Errorf("lock task %s: %w", id, err) } @@ -629,7 +702,7 @@ func (s *Store) write(state *State) error { if err := tmp.Close(); err != nil { return fmt.Errorf("close temp task file: %w", err) } - if err := os.Rename(tmpPath, target); err != nil { + if err := atomicfile.Rename(tmpPath, target); err != nil { return fmt.Errorf("commit task state: %w", err) } syncDir(s.dir) @@ -645,3 +718,30 @@ func syncDir(dir string) { defer func() { _ = d.Close() }() _ = d.Sync() } + +func (s *Store) create(opts CreateOptions, launch bool) (*Task, error) { + id := random.String(12) + t := &Task{store: s, id: id} + if launch { + if err := t.claimLaunchOwnership(); err != nil { + return nil, err + } + } + now := time.Now() + state := &State{ + ID: id, + Command: opts.Command, + WorkspaceID: opts.WorkspaceID, + Status: StatusPending, + StartedAt: now, + UpdatedAt: now, + } + if launch { + state.LaunchPending = true + state.LaunchProtocol = 1 + } + if err := s.withLock(id, func() error { return s.write(state) }); err != nil { + return nil, errors.Join(err, releaseLauncherLock(t)) + } + return t, nil +} diff --git a/pkg/task/store_test.go b/pkg/task/store_test.go index 8e3de69fde..f314ed7f29 100644 --- a/pkg/task/store_test.go +++ b/pkg/task/store_test.go @@ -3,6 +3,8 @@ package task import ( "context" "errors" + "os" + "sync" "testing" "time" @@ -14,6 +16,53 @@ const workspaceOne = "ws-1" const testProcessIdentity = "sess-1" +func TestGetWaitsForWriterWhenRecordTemporarilyMissing(t *testing.T) { + assertTaskReadWaitsForWriter(t, func(path string) func() { + previous := path + ".previous" + require.NoError(t, os.Rename(path, previous)) + return func() { require.NoError(t, os.Rename(previous, path)) } + }) +} + +func assertTaskReadWaitsForWriter(t *testing.T, interruptRead func(string) func()) { + t.Helper() + writer := newTestStore(t) + reader, err := NewStoreAt(writer.dir) + require.NoError(t, err) + tk, err := writer.Create(CreateOptions{}) + require.NoError(t, err) + state, err := writer.Get(tk.ID()) + require.NoError(t, err) + path, err := writer.path(tk.ID()) + require.NoError(t, err) + blocked := make(chan struct{}) + reader.afterStateLockBlockedHook = func() { close(blocked) } + type readResult struct { + state *State + err error + } + read := make(chan readResult, 1) + require.NoError(t, writer.withLock(tk.ID(), func() error { + restore := sync.OnceFunc(interruptRead(path)) + defer restore() + go func() { + state, err := reader.Get(tk.ID()) + read <- readResult{state, err} + }() + select { + case <-blocked: + case result := <-read: + t.Fatalf("reader bypassed active writer: state=%+v error=%v", result.state, result.err) + } + restore() + state.Phase = "replacement-committed" + return writer.write(state) + })) + result := <-read + require.NoError(t, result.err) + require.Equal(t, "replacement-committed", result.state.Phase) +} + func createTaskWith(t *testing.T, store *Store, opts CreateOptions) *Task { t.Helper() tk, err := store.Create(opts) @@ -454,3 +503,67 @@ func TestStateNeedsReconciliation(t *testing.T) { }) } } + +func TestTaskQueriesRejectUncertainRecords(t *testing.T) { + for _, failure := range []string{"state-lock", "malformed-state"} { + t.Run(failure, func(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{WorkspaceID: workspaceOne, Command: "up"}) + queries := map[string]func() ([]*State, error){ + "List": store.List, + "ForWorkspace": func() ([]*State, error) { return store.ForWorkspace(workspaceOne, "up") }, + "ActiveForWorkspace": func() ([]*State, error) { return store.ActiveForWorkspace(workspaceOne, "up") }, + } + check := func() error { + for name, query := range queries { + t.Run(name, func(t *testing.T) { + states, err := query() + require.Error(t, err) + require.Nil(t, states) + }) + } + return nil + } + if failure == "state-lock" { + store.stateLockTimeout = -time.Nanosecond + require.NoError(t, store.withLock(tk.ID(), check)) + } else { + path, err := store.path(tk.ID()) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, []byte("{"), 0o600)) + require.NoError(t, check()) + } + }) + } +} + +func TestListOmitsRecordDeletedWhileWaitingForStateLock(t *testing.T) { + writer := newTestStore(t) + reader, err := NewStoreAt(writer.dir) + require.NoError(t, err) + tk := createTaskWith(t, writer, CreateOptions{}) + path, err := writer.path(tk.ID()) + require.NoError(t, err) + blocked := make(chan struct{}) + reader.afterStateLockBlockedHook = func() { close(blocked) } + type result struct { + states []*State + err error + } + read := make(chan result, 1) + require.NoError(t, writer.withLock(tk.ID(), func() error { + go func() { + states, err := reader.List() + read <- result{states, err} + }() + select { + case <-blocked: + case r := <-read: + t.Fatalf("listing bypassed writer: %+v", r) + } + return os.Remove(path) + })) + r := <-read + require.NoError(t, r.err) + require.Empty(t, r.states) +} diff --git a/pkg/task/task.go b/pkg/task/task.go index c797a25cb3..4bc6da26e2 100644 --- a/pkg/task/task.go +++ b/pkg/task/task.go @@ -66,6 +66,10 @@ type State struct { ProcessCleanupComplete bool `json:"processCleanupComplete,omitempty"` CancelRequested bool `json:"cancelRequested,omitempty"` LaunchPending bool `json:"launchPending,omitempty"` + LaunchProtocol int `json:"launchProtocol,omitempty"` + WorkerReady bool `json:"workerReady,omitempty"` + LaunchTreeID string `json:"launchTreeId,omitempty"` + LaunchOwned bool `json:"launchOwned,omitempty"` StartedAt time.Time `json:"startedAt"` UpdatedAt time.Time `json:"updatedAt"` } @@ -85,6 +89,7 @@ type Task struct { // Held for the worker process's lifetime; see HoldWorkerLock. workerLock *flock.Flock launcherLock *flock.Flock + launchLease *command.LaunchLease } func (t *Task) ID() string { return t.id } @@ -141,30 +146,25 @@ func (s *State) NeedsReconciliation() bool { } func (t *Task) BeginLaunch() error { - launcherLock, available, err := t.store.tryLauncherLock(t.id) - if err != nil { - return fmt.Errorf("lock task %s launcher: %w", t.id, err) - } - if !available { - return fmt.Errorf("task %s already has a launcher", t.id) + if err := t.claimLaunchOwnership(); err != nil { + return err } var canceled bool - err = t.store.update(t.id, func(s *State) { - if s.Status.Terminal() || s.CancelRequested { + err := t.store.update(t.id, func(s *State) { + if s.Status.Terminal() || s.CancelRequested || s.LaunchProtocol != 0 || s.PID != 0 { canceled = true return } s.LaunchPending = true + s.LaunchProtocol = 1 + s.WorkerReady = false }) if err != nil { - _ = launcherLock.Unlock() - return err + return errors.Join(err, releaseLauncherLock(t)) } if canceled { - _ = launcherLock.Unlock() - return ErrCanceled + return errors.Join(ErrCanceled, releaseLauncherLock(t)) } - t.launcherLock = launcherLock return nil } @@ -237,15 +237,20 @@ func (t *Task) ReleaseWorkerLock() error { } func releaseLauncherLock(t *Task) error { + var leaseErr error + if t.launchLease != nil { + leaseErr = t.launchLease.Close() + t.launchLease = nil + } if t.launcherLock == nil { - return nil + return leaseErr } lock := t.launcherLock t.launcherLock = nil if err := lock.Unlock(); err != nil { return fmt.Errorf("unlock task %s launcher: %w", t.id, err) } - return nil + return leaseErr } // SetWorkspaceID corrects the task's workspace label to the resolved ID, @@ -470,23 +475,15 @@ func waitForLaunchStep(t *Task) (*State, bool, error) { } func clearAbandonedLaunch(t *Task) (bool, error) { - launcherLock, available, err := t.store.tryLauncherLock(t.id) + state, err := t.store.Get(t.id) if err != nil { - return false, fmt.Errorf("cancel task %s: inspect launcher lock: %w", t.id, err) + return false, err } - if !available { - return false, nil - } - defer func() { _ = launcherLock.Unlock() }() - err = t.store.update(t.id, func(current *State) { - if current.CancelRequested && current.LaunchPending { - current.LaunchPending = false - } - }) - if err != nil { - return false, fmt.Errorf("cancel task %s: clear abandoned launch: %w", t.id, err) + if state.LaunchProtocol == 1 { + current, err := t.store.reconcilePendingLaunch(context.Background(), t.id) + return current != nil && !current.LaunchPending, err } - return true, nil + return false, errors.New("cannot verify cleanup of a legacy abandoned launch") } func waitForCancellation(ctx context.Context) error { @@ -511,6 +508,10 @@ func markCanceled(s *State) { // Fail preserves an existing terminal state, so the error a canceled worker // reports on its way out doesn't mask ErrCanceled as the reason it stopped. func (t *Task) Fail(err error) error { + return t.recordFailure(err, false) +} + +func (t *Task) recordFailure(err error, failedLaunch bool) error { message := "" var classified *clierr.CLIError if err != nil { @@ -527,6 +528,9 @@ func (t *Task) Fail(err error) error { } redactor := secrets.NewEnvironmentRedactor(os.Environ()) return t.store.update(t.id, func(s *State) { + if failedLaunch { + s.markLaunchCleanupPending() + } if s.Status.Terminal() || s.CancelRequested { return } diff --git a/pkg/task/task_test.go b/pkg/task/task_test.go index d940baca5b..8e18785675 100644 --- a/pkg/task/task_test.go +++ b/pkg/task/task_test.go @@ -1285,6 +1285,12 @@ func TestCancelUnpublishedPIDStaysRetryable(t *testing.T) { t.Fatalf("BeginLaunch: %v", err) } + t.Cleanup(func() { + if err := releaseLauncherLock(tk); err != nil { + t.Errorf("release launcher: %v", err) + } + }) + setKillProcessForTest(store, tk, func(pid, treeName string) error { t.Error("kill invoked before the worker published a pid") return nil diff --git a/pkg/workspace/quiesce.go b/pkg/workspace/quiesce.go index 7b4ad9a0cf..9393f451d3 100644 --- a/pkg/workspace/quiesce.go +++ b/pkg/workspace/quiesce.go @@ -41,7 +41,7 @@ func quiesceUpTasks( var errs []error handled := 0 for _, state := range states { - if state.Status.Terminal() && !state.NeedsExitedWorkerCleanup() && + if state.Status.Terminal() && !state.LaunchPending && !state.NeedsExitedWorkerCleanup() && !state.NeedsCanceledWorkerCleanup() { continue } diff --git a/pkg/workspace/quiesce_test.go b/pkg/workspace/quiesce_test.go index 2b07761214..30853e120f 100644 --- a/pkg/workspace/quiesce_test.go +++ b/pkg/workspace/quiesce_test.go @@ -3,6 +3,8 @@ package workspace import ( "context" "errors" + "os" + "path/filepath" "runtime" "testing" @@ -160,3 +162,32 @@ func TestQuiesceUpTasksBoundsEachCancellation(t *testing.T) { ) require.NoError(t, err) } + +func TestQuiesceUpTasksRejectsUnverifiedTerminalLaunch(t *testing.T) { + dir := t.TempDir() + store, err := task.NewStoreAt(dir) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dir, "interrupted.json"), []byte( + `{"id":"interrupted","command":"up","workspaceId":"ws-1","status":"failed","launchPending":true}`, + ), 0o600)) + + err = QuiesceUpTasks(store, "ws-1") + require.ErrorContains(t, err, "cannot verify cleanup of a legacy abandoned launch") + assert.True(t, taskState(t, store, "interrupted").LaunchPending) +} + +func TestQuiesceUpTasksRejectsMalformedTaskRecord(t *testing.T) { + dir := t.TempDir() + store, err := task.NewStoreAt(dir) + require.NoError(t, err) + tk := createUpTask(t, store, "ws-1") + path := filepath.Join(dir, tk.ID()+".json") + require.NoError(t, os.WriteFile(path, []byte("{"), 0o600)) + called := false + err = quiesceUpTasks(context.Background(), store, "ws-1", func(context.Context, string) error { + called = true + return nil + }) + require.ErrorContains(t, err, "list up tasks for workspace ws-1") + require.False(t, called) +} diff --git a/pkg/workspace/workspace.go b/pkg/workspace/workspace.go index 74af72b513..a2f9349474 100644 --- a/pkg/workspace/workspace.go +++ b/pkg/workspace/workspace.go @@ -71,7 +71,7 @@ func Resolve( devsyConfig *config.Config, params ResolveParams, ) (client.BaseWorkspaceClient, error) { - if err := validateDesiredID(params.DesiredID); err != nil { + if err := ValidateID(params.DesiredID); err != nil { return nil, err } @@ -115,7 +115,8 @@ func Resolve( return workspaceClient, nil } -func validateDesiredID(desiredID string) error { +// ValidateID checks an explicit workspace ID; empty leaves ID derivation to the source. +func ValidateID(desiredID string) error { if desiredID == "" { return nil } diff --git a/pkg/workspace/workspace_test.go b/pkg/workspace/workspace_test.go index 5c4e45b2af..2124f28b1e 100644 --- a/pkg/workspace/workspace_test.go +++ b/pkg/workspace/workspace_test.go @@ -21,7 +21,7 @@ const ( testDevContainerPath = ".devcontainer/devcontainer.json" ) -func TestValidateDesiredID(t *testing.T) { +func TestValidateID(t *testing.T) { cases := []struct { name string desiredID string @@ -45,7 +45,7 @@ func TestValidateDesiredID(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - err := validateDesiredID(c.desiredID) + err := ValidateID(c.desiredID) if c.wantErr { require.Error(t, err) } else { @@ -55,10 +55,10 @@ func TestValidateDesiredID(t *testing.T) { } } -func TestValidateDesiredID_InvalidCharsCheckedBeforeLength(t *testing.T) { +func TestValidateID_InvalidCharsCheckedBeforeLength(t *testing.T) { // An over-length id that also contains invalid characters should report the // character rule first, matching the historical ordering. - err := validateDesiredID(strings.Repeat("A", maxWorkspaceIDLength+1)) + err := ValidateID(strings.Repeat("A", maxWorkspaceIDLength+1)) require.Error(t, err) assert.Contains(t, err.Error(), "lowercase letters") } diff --git a/prek.toml b/prek.toml index 5683a7aeb7..ace6fa7185 100644 --- a/prek.toml +++ b/prek.toml @@ -64,7 +64,7 @@ hooks = [ # Tracked upstream: https://github.com/rhysd/actionlint/issues/693 [[repos]] repo = "https://github.com/rhysd/actionlint" -rev = "v1.7.12" +rev = "fd33e9f582a01a02885c93ae3775e1190cf63653" hooks = [ { id = "actionlint", diff --git a/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx b/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx index c2cabcea26..a93de3ea05 100644 --- a/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx +++ b/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx @@ -25,6 +25,20 @@ Open **Workspaces** and click **Create**. The wizard has five steps: The desktop app runs `devsy workspace up` for you. +### Workspace IDs + +The workspace name is its ID. Use lowercase ASCII letters (`a-z`), digits (`0-9`), and hyphens (`-`), with a maximum of 48 characters. + +Desktop suggests a valid ID from your source. For example, a local folder named `Backend` suggests `backend`; the source path keeps its original case. If you enter a name yourself, it must follow the same rules: + +```sh +devsy workspace up ./Projects/Backend --id backend +``` + + +Uppercase workspace IDs are rejected before detached creation starts. Startup failures are recorded as failed tasks and shown in Desktop instead of leaving creation waiting indefinitely. Mixed-case source folder and repository names remain supported. + + ### Devsy CLI [Install the CLI](../getting-started/install.mdx#install-devsy-cli) and add a provider, for example Docker: