From 6a3b400d8ae479c16dc7e27034c642ec73ec288a Mon Sep 17 00:00:00 2001 From: pratikbin <68642400+pratikbin@users.noreply.github.com> Date: Fri, 28 Aug 2026 10:02:54 +0530 Subject: [PATCH 1/3] feat(sandbox): add offload, matrix and self commands with safety guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce `sandbox offload` and `sandbox matrix` subcommands for one‑shot offload and parallel fork workflows. Add `sandbox self` for in‑sandbox signaling. Implement guards, retry logic, and improved error handling for lifecycle, connection setup, and disk operations. --- .secrets.baseline | 18 +- cmd/root/root.go | 11 + cmd/root/usage_error.go | 169 +++++++++++ cmd/root/usage_error_test.go | 113 +++++++ cmd/sandbox/compose.go | 273 +++++++++++++++++ cmd/sandbox/compose_test.go | 303 +++++++++++++++++++ cmd/sandbox/create.go | 1 + cmd/sandbox/edit.go | 1 + cmd/sandbox/egress_preset.go | 76 +++++ cmd/sandbox/fork.go | 223 ++++++++++++-- cmd/sandbox/guards.go | 91 ++++++ cmd/sandbox/guards_test.go | 125 ++++++++ cmd/sandbox/lifecycle_test.go | 342 +++++++++++++++++++++ cmd/sandbox/matrix.go | 474 ++++++++++++++++++++++++++++++ cmd/sandbox/offload.go | 343 +++++++++++++++++++++ cmd/sandbox/pull.go | 8 + cmd/sandbox/push.go | 7 + cmd/sandbox/sandbox.go | 3 + cmd/sandbox/self.go | 206 +++++++++++++ cmd/sandbox/stage.go | 287 ++++++++++++++++++ internal/api/client.go | 73 ++++- internal/api/client_retry_test.go | 117 ++++++++ internal/api/sandbox.go | 15 +- internal/api/sandbox_client.go | 1 + 24 files changed, 3240 insertions(+), 40 deletions(-) create mode 100644 cmd/root/usage_error.go create mode 100644 cmd/root/usage_error_test.go create mode 100644 cmd/sandbox/compose.go create mode 100644 cmd/sandbox/compose_test.go create mode 100644 cmd/sandbox/egress_preset.go create mode 100644 cmd/sandbox/guards.go create mode 100644 cmd/sandbox/guards_test.go create mode 100644 cmd/sandbox/lifecycle_test.go create mode 100644 cmd/sandbox/matrix.go create mode 100644 cmd/sandbox/offload.go create mode 100644 cmd/sandbox/self.go create mode 100644 cmd/sandbox/stage.go create mode 100644 internal/api/client_retry_test.go diff --git a/.secrets.baseline b/.secrets.baseline index 55812ed..b15bca8 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -90,6 +90,10 @@ { "path": "detect_secrets.filters.allowlist.is_line_allowlisted" }, + { + "path": "detect_secrets.filters.common.is_baseline_file", + "filename": ".secrets.baseline" + }, { "path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies", "min_level": 2 @@ -129,8 +133,8 @@ "filename": "cmd/env/set.go", "hashed_secret": "ec417f567082612f8fd6afafe1abcab831fca840", "is_verified": false, - "is_secret": false, - "line_number": 24 + "line_number": 24, + "is_secret": false } ], "cmd/oauth/helpers.go": [ @@ -139,8 +143,8 @@ "filename": "cmd/oauth/helpers.go", "hashed_secret": "a587be0a364eab71821821cfc5226eb04853224f", "is_verified": false, - "is_secret": false, - "line_number": 155 + "line_number": 155, + "is_secret": false } ], "internal/api/client.go": [ @@ -149,10 +153,10 @@ "filename": "internal/api/client.go", "hashed_secret": "b19a5a3616bf8b53864ca6162b5f1f6a8c61ab94", "is_verified": false, - "is_secret": false, - "line_number": 97 + "line_number": 222, + "is_secret": false } ] }, - "generated_at": "2026-04-02T07:26:00Z" + "generated_at": "2026-08-28T04:32:28Z" } diff --git a/cmd/root/root.go b/cmd/root/root.go index 8a21cdc..84a2944 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -108,6 +108,15 @@ func NewApp() *cli.App { if cmd == "" || cmd == "login" || cmd == "logout" || cmd == "version" || cmd == "ask" || cmd == "upgrade" { return nil } + // `sandbox self` talks to the guest agent on loopback inside the + // sandbox, which takes no credential by design. Demanding a + // login here would make the command unusable exactly where it is + // meant to run: inside a sandbox, which has no stored token. + if cmd == "sandbox" || cmd == "sb" { + if sub := c.Args().Get(1); sub == "self" { + return nil + } + } // CREATEOS_API_KEY env var (or --api-key flag) — injected by Stripe Projects if apiKey := c.String("api-key"); apiKey != "" { @@ -228,6 +237,8 @@ func NewApp() *cli.App { }, } installTrailingHelpGuards(app.Commands) + installUsageErrorHelp(app) + installCommandSuggestions(app) return app } diff --git a/cmd/root/usage_error.go b/cmd/root/usage_error.go new file mode 100644 index 0000000..2098537 --- /dev/null +++ b/cmd/root/usage_error.go @@ -0,0 +1,169 @@ +package root + +import ( + "fmt" + "os" + "strings" + + "github.com/urfave/cli/v2" +) + +// urfave/cli parses global flags only before the first command name, so +// `createos sandbox shapes -o json` dies with "flag provided but not +// defined: -o" — the flag exists, it is simply in the wrong place. The +// message names neither fact, and the same shape has cost real round trips +// in practice. +// +// installUsageErrorHelp attaches the handler to every command in the tree. +// OnUsageError lives on Command and is NOT inherited from the app, so a +// handler set only at the top never runs for `sandbox shapes -o json` — +// the exact case worth catching. +func installUsageErrorHelp(app *cli.App) { + globals := globalFlagNames(app) + handler := func(_ *cli.Context, err error, _ bool) error { + name := undefinedFlagName(err) + if name == "" || !globals[strings.TrimLeft(name, "-")] { + return err + } + return fmt.Errorf("%w\n\n %s is a global flag, so it has to come BEFORE the command:\n %s", + err, name, correctedCommandLine(name)) + } + app.OnUsageError = handler + setUsageErrorHandler(app.Commands, handler) +} + +func setUsageErrorHandler(commands []*cli.Command, handler cli.OnUsageErrorFunc) { + for _, cmd := range commands { + if cmd == nil { + continue + } + if cmd.OnUsageError == nil { + cmd.OnUsageError = handler + } + setUsageErrorHandler(cmd.Subcommands, handler) + } +} + +func globalFlagNames(app *cli.App) map[string]bool { + names := make(map[string]bool) + for _, f := range app.Flags { + for _, n := range f.Names() { + names[n] = true + } + } + return names +} + +// undefinedFlagName pulls the flag out of the flag package's message, +// which reads: `flag provided but not defined: -o`. +func undefinedFlagName(err error) string { + const marker = "flag provided but not defined: " + msg := err.Error() + i := strings.Index(msg, marker) + if i < 0 { + return "" + } + name := strings.TrimSpace(msg[i+len(marker):]) + if cut := strings.IndexAny(name, " \n"); cut >= 0 { + name = name[:cut] + } + return name +} + +// correctedCommandLine rewrites what the user typed with the misplaced +// global flag moved to the front, so the fix can be copied straight back +// into the terminal. +func correctedCommandLine(flagName string) string { + bare := strings.TrimLeft(flagName, "-") + args := os.Args[1:] + + moved := make([]string, 0, 2) + rest := make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + a := args[i] + trimmed := strings.TrimLeft(a, "-") + if trimmed == bare || strings.HasPrefix(trimmed, bare+"=") { + moved = append(moved, a) + // A value-taking flag written as `-o json` carries its value + // in the next argument; move that too or the corrected line + // is wrong. + if !strings.Contains(a, "=") && i+1 < len(args) && !strings.HasPrefix(args[i+1], "-") { + i++ + moved = append(moved, args[i]) + } + continue + } + rest = append(rest, a) + } + if len(moved) == 0 { + return "createos " + strings.Join(args, " ") + } + return "createos " + strings.Join(append(moved, rest...), " ") +} + +// installCommandSuggestions replaces urfave's bare "No help topic for +// 'ssh'" with the nearest real command. Agents and people both guess verb +// names, and a guess that lands one edit away from a real command should +// not cost a round trip to the help output. +func installCommandSuggestions(app *cli.App) { + app.CommandNotFound = func(c *cli.Context, name string) { + // The hook fires for a subcommand too ("createos sandbox ssh"), and + // there the useful candidates are that group's subcommands, not the + // top-level verbs. Searching the wrong list is worse than staying + // quiet: it points at something unrelated. + candidates, prefix := app.Commands, "" + if cmd := c.Command; cmd != nil && len(cmd.Subcommands) > 0 { + candidates, prefix = cmd.Subcommands, cmd.Name+" " + } + noun := "command" + if prefix != "" { + noun = "subcommand" + } + fmt.Fprintf(os.Stderr, "createos %s: %q is not a %s.\n", strings.TrimSpace(prefix), name, noun) + if best := nearestCommand(candidates, name); best != "" { + fmt.Fprintf(os.Stderr, "\n Did you mean:\n createos %s%s\n", prefix, best) + } + fmt.Fprintf(os.Stderr, "\n See everything with:\n createos %s--help\n", prefix) + } +} + +// nearestCommand returns the closest command name within a small edit +// distance, or "" when nothing is close enough. The cap matters: a wild +// guess should get the help pointer, not a confidently wrong suggestion. +func nearestCommand(commands []*cli.Command, name string) string { + name = strings.ToLower(name) + best, bestDist := "", 3 + for _, cmd := range commands { + if cmd.Hidden { + continue + } + for _, candidate := range append([]string{cmd.Name}, cmd.Aliases...) { + if d := editDistance(name, strings.ToLower(candidate)); d < bestDist { + best, bestDist = cmd.Name, d + } + } + } + return best +} + +// editDistance is Levenshtein over two short command names, with one row +// of state rather than a full matrix. +func editDistance(a, b string) int { + prev := make([]int, len(b)+1) + for j := range prev { + prev[j] = j + } + for i := 1; i <= len(a); i++ { + cur := make([]int, len(b)+1) + cur[0] = i + for j := 1; j <= len(b); j++ { + cost := 1 + if a[i-1] == b[j-1] { + cost = 0 + } + cur[j] = min(prev[j]+1, min(cur[j-1]+1, prev[j-1]+cost)) + } + prev = cur + } + return prev[len(b)] +} diff --git a/cmd/root/usage_error_test.go b/cmd/root/usage_error_test.go new file mode 100644 index 0000000..3e044a6 --- /dev/null +++ b/cmd/root/usage_error_test.go @@ -0,0 +1,113 @@ +package root + +import ( + "errors" + "os" + "testing" + + "github.com/urfave/cli/v2" +) + +func TestUndefinedFlagName(t *testing.T) { + for _, tc := range []struct { + err error + want string + }{ + {errors.New("flag provided but not defined: -o"), "-o"}, + {errors.New("flag provided but not defined: --output"), "--output"}, + {errors.New("something else entirely"), ""}, + } { + if got := undefinedFlagName(tc.err); got != tc.want { + t.Errorf("undefinedFlagName(%q) = %q, want %q", tc.err, got, tc.want) + } + } +} + +// TestCorrectedCommandLine covers the payoff: the message has to hand back +// a line the user can paste, which means moving the flag AND its value. +func TestCorrectedCommandLine(t *testing.T) { + for _, tc := range []struct { + name string + args []string + flag string + want string + }{ + { + "separate value moves with the flag", + []string{"sandbox", "shapes", "-o", "json"}, + "-o", + "createos -o json sandbox shapes", + }, + { + "joined value", + []string{"sandbox", "shapes", "--output=json"}, + "--output", + "createos --output=json sandbox shapes", + }, + { + "boolean flag has no value to move", + []string{"sandbox", "ls", "--debug"}, + "--debug", + "createos --debug sandbox ls", + }, + { + "flag already first is left alone", + []string{"-o", "json", "sandbox", "shapes"}, + "-o", + "createos -o json sandbox shapes", + }, + } { + t.Run(tc.name, func(t *testing.T) { + original := os.Args + os.Args = append([]string{"createos"}, tc.args...) + t.Cleanup(func() { os.Args = original }) + + if got := correctedCommandLine(tc.flag); got != tc.want { + t.Errorf("correctedCommandLine() = %q, want %q", got, tc.want) + } + }) + } +} + +// TestNearestCommand pins both halves: a near miss gets a suggestion, and +// a wild guess gets silence. Suggesting something unrelated is worse than +// suggesting nothing. +func TestNearestCommand(t *testing.T) { + commands := []*cli.Command{ + {Name: "shell", Aliases: []string{"sh"}}, + {Name: "fork"}, + {Name: "exec"}, + {Name: "hidden", Hidden: true}, + } + for _, tc := range []struct { + input string + want string + }{ + {"ssh", "shell"}, // one edit from the "sh" alias + {"forkk", "fork"}, // one extra character + {"exe", "exec"}, // one missing character + {"zzzzqqq", ""}, // nothing close — stay quiet + {"hidden", ""}, // hidden commands are not suggested + } { + if got := nearestCommand(commands, tc.input); got != tc.want { + t.Errorf("nearestCommand(%q) = %q, want %q", tc.input, got, tc.want) + } + } +} + +func TestEditDistance(t *testing.T) { + for _, tc := range []struct { + a, b string + want int + }{ + {"", "", 0}, + {"fork", "fork", 0}, + {"forkk", "fork", 1}, + {"ssh", "sh", 1}, + {"exec", "", 4}, + } { + if got := editDistance(tc.a, tc.b); got != tc.want { + t.Errorf("editDistance(%q, %q) = %d, want %d", tc.a, tc.b, got, tc.want) + } + } +} diff --git a/cmd/sandbox/compose.go b/cmd/sandbox/compose.go new file mode 100644 index 0000000..c56c189 --- /dev/null +++ b/cmd/sandbox/compose.go @@ -0,0 +1,273 @@ +package sandbox + +import ( + "context" + "encoding/base64" + "errors" + "fmt" + "io" + "strings" + "time" + + "github.com/urfave/cli/v2" + + "github.com/NodeOps-app/createos-cli/internal/api" +) + +// The composed verbs (offload, matrix) share one box recipe. Keeping the +// flags in one place is what stops `offload --shape` and `matrix --shape` +// from drifting apart. +const ( + composeDefaultShape = "s-1vcpu-1gb" + composeDefaultRootfs = "devbox:1" + // composeDefaultAutoPause is the backstop. If this CLI is killed + // mid-run — a closed laptop, a dropped SSH session, a CI timeout — + // nothing is left to destroy the boxes it created, and they bill + // until someone notices. Auto-pause makes the sandbox park itself. + composeDefaultAutoPause = 15 * time.Minute + // composeWorkDir is where a staged tree lands inside the sandbox. + composeWorkDir = "/work" +) + +// composeFlags are the box-shape flags shared by offload and matrix. +func composeFlags() []cli.Flag { + return []cli.Flag{ + &cli.StringFlag{Name: "shape", Value: composeDefaultShape, Usage: "Sandbox size (run 'createos sandbox shapes' to see options)"}, + &cli.StringFlag{Name: "rootfs", Value: composeDefaultRootfs, Usage: "Base image or template to boot from"}, + &cli.StringSliceFlag{Name: "env", Usage: "Environment variable for every command (repeatable): KEY=VALUE"}, + &cli.StringSliceFlag{Name: "egress", Usage: "Host the sandbox may reach (repeatable). Default: unrestricted"}, + &cli.StringSliceFlag{Name: "egress-preset", Usage: "Toolchain allowlist (repeatable): " + strings.Join(egressPresetNames(), ", ")}, + &cli.StringSliceFlag{Name: "exclude", Usage: "Path to keep out of the upload (repeatable). .gitignore is honoured already"}, + &cli.DurationFlag{Name: "auto-pause", Value: composeDefaultAutoPause, Usage: "Park an idle sandbox if this command dies before it can clean up. 0 disables"}, + &cli.DurationFlag{Name: "timeout", Usage: "Give up on a command after this long. Default: no limit"}, + } +} + +// composeOptions is composeFlags after parsing and validation. +type composeOptions struct { + Shape string + Rootfs string + Env map[string]string + Egress []string + Exclude []string + AutoPause time.Duration + Timeout time.Duration +} + +func parseComposeOptions(c *cli.Context) (*composeOptions, error) { + env, err := parseKeyValues(c.StringSlice("env")) + if err != nil { + return nil, err + } + egress, err := resolveEgress(c.StringSlice("egress-preset"), c.StringSlice("egress")) + if err != nil { + return nil, err + } + return &composeOptions{ + Shape: c.String("shape"), + Rootfs: c.String("rootfs"), + Env: env, + Egress: egress, + Exclude: c.StringSlice("exclude"), + AutoPause: c.Duration("auto-pause"), + Timeout: c.Duration("timeout"), + }, nil +} + +// checkMisplacedFlags turns a silent drop into a clear error. +// +// urfave/cli stops parsing flags at the first positional argument, so +// `matrix . --job 'x'` parses `.` and then treats --job as a plain +// argument: the job list comes back empty and nothing says why. This is a +// standing trap in this CLI — the same shape already bit `process run +// --cwd` (commit 8c1f7ac) and `sandbox shapes -o json`. +// +// A user cannot be expected to know where the parser gave up, so any +// leftover argument that names one of this command's own flags is +// reported with the corrected command line. +func checkMisplacedFlags(c *cli.Context, leftovers []string) error { + known := make(map[string]bool) + for _, f := range c.Command.Flags { + for _, n := range f.Names() { + known["--"+n] = true + known["-"+n] = true + } + } + for _, arg := range leftovers { + name, _, _ := strings.Cut(arg, "=") + if known[name] { + return fmt.Errorf( + "%s was written after the directory, so it was ignored\n\n Flags must come before the directory:\n createos sandbox %s %s ", + name, c.Command.Name, name) + } + } + return nil +} + +// parseKeyValues turns repeated KEY=VALUE flags into a map. +func parseKeyValues(pairs []string) (map[string]string, error) { + if len(pairs) == 0 { + return nil, nil + } + out := make(map[string]string, len(pairs)) + for _, p := range pairs { + k, v, ok := strings.Cut(p, "=") + if !ok || k == "" { + return nil, fmt.Errorf("--env %q is not KEY=VALUE", p) + } + out[k] = v + } + return out, nil +} + +// createComposeBox boots one sandbox to the shared recipe and waits for it +// to run. +func createComposeBox(ctx context.Context, client *api.SandboxClient, opts *composeOptions) (*api.SandboxView, error) { + // No name: these boxes are machinery with a lifetime of one command, + // and a generated name is easier to tell apart in `sandbox ls` than a + // dozen boxes all called the same thing. + req := api.SandboxCreateReq{ + Shape: opts.Shape, + Rootfs: opts.Rootfs, + Egress: opts.Egress, + Envs: opts.Env, + } + if opts.AutoPause > 0 { + secs := int(opts.AutoPause.Seconds()) + req.AutoPauseAfterSeconds = &secs + } + created, err := client.CreateSandbox(ctx, req) + if err != nil { + return nil, err + } + // Past this point the sandbox exists and is billable. Every way out + // that is not "running" has to destroy it, or a readiness timeout + // leaves a machine nobody knows about — offload and matrix only see + // the error, never the id. + sb, err := waitForStatus(ctx, client, created.ID, "running") + if err != nil { + return nil, cleanupAfterCreate(ctx, client, created.ID, err) + } + if sb.Status != "running" { + return nil, cleanupAfterCreate(ctx, client, sb.ID, + fmt.Errorf("sandbox %s came up %s, not running", sb.ID, sb.Status)) + } + return sb, nil +} + +// cleanupAfterCreate destroys a sandbox that never became usable and folds +// the outcome into the error the caller sees. The id is always named: if +// the teardown itself fails, the user needs it to clean up by hand. +func cleanupAfterCreate(ctx context.Context, client *api.SandboxClient, id string, cause error) error { + tearCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) + defer cancel() + if err := client.DestroySandbox(tearCtx, id); err != nil { + return fmt.Errorf("%w\n\n Sandbox %s was created and could not be destroyed (%w).\n It is still billable. Remove it with:\n createos sandbox rm --force %s", + cause, id, err, id) + } + return fmt.Errorf("%w\n\n Sandbox %s was destroyed", cause, id) +} + +// managedResult is one command's outcome. +type managedResult struct { + ExitCode int + Signal string + Duration time.Duration +} + +// composeStreamRetries bounds how many times runManaged reconnects to a +// live process after the output stream drops. +const composeStreamRetries = 5 + +// runManaged runs cmd as a managed process and streams its output to out. +// +// A managed process is the point. `sandbox exec` ties the command's life +// to the HTTP stream, so a connection that drops on a long quiet build — +// a laptop sleeping, a proxy idle-timeout — kills the remote command. A +// managed process keeps running on the box and keeps replayable output, so +// this function reconnects from the last sequence it saw and picks the +// output back up. That reconnect is the whole reason the composed verbs +// survive builds that print nothing for ten minutes. +func runManaged( + ctx context.Context, + client *api.SandboxClient, + sandboxID, cmd, cwd string, + env map[string]string, + out io.Writer, +) (*managedResult, error) { + start := time.Now() + proc, err := client.CreateProcess(ctx, sandboxID, api.ProcessCreateRequest{ + Cmd: "bash", + Args: []string{"-lc", cmd}, + Cwd: cwd, + Env: env, + }) + if err != nil { + return nil, fmt.Errorf("start command in %s: %w", sandboxID, err) + } + + var exitCode *int + var signal string + after := int64(0) + + for attempt := 0; ; attempt++ { + streamErr := client.ConnectProcess(ctx, sandboxID, proc.ProcessID, after, func(ev api.ProcessOutputEvent) { + switch ev.Type { + case "data": + if ev.Seq > after { + after = ev.Seq + } + if raw, decErr := base64.StdEncoding.DecodeString(ev.DataBase64); decErr == nil { + _, _ = out.Write(raw) //nolint:errcheck // a failed log write must not kill the job + } + case "exit": + exitCode = ev.ExitCode + signal = ev.Signal + } + }) + if exitCode != nil { + break + } + if ctx.Err() != nil { + return nil, ctx.Err() + } + if streamErr == nil || errors.Is(streamErr, context.Canceled) { + // Stream ended cleanly but no exit frame arrived. Ask the + // server directly rather than guessing. + done, waitErr := client.WaitProcess(ctx, sandboxID, proc.ProcessID, true, int64(pollTimeout/time.Millisecond)) + if waitErr != nil { + return nil, waitErr + } + if done.ExitCode != nil { + exitCode = done.ExitCode + signal = done.Signal + break + } + } + if attempt >= composeStreamRetries { + return nil, fmt.Errorf("lost the output stream for %s in %s after %d reconnects: %w", + proc.ProcessID, sandboxID, attempt, streamErr) + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(time.Second): + } + } + + return &managedResult{ExitCode: *exitCode, Signal: signal, Duration: time.Since(start)}, nil +} + +// destroyQuiet tears a sandbox down and reports failures without stopping +// the caller. A composed verb is usually already unwinding when it calls +// this, and a teardown error must not mask the real one — but it must not +// be swallowed either, because the sandbox is still billable. +func destroyQuiet(ctx context.Context, client *api.SandboxClient, id string, warn func(string)) { + // The caller's context may already be cancelled (Ctrl-C, timeout). + // Teardown still has to happen, so give it a context of its own. + tearCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) + defer cancel() + if err := client.DestroySandbox(tearCtx, id); err != nil && warn != nil { + warn(fmt.Sprintf("could not destroy %s: %v — remove it with: createos sandbox rm --force %s", id, err, id)) + } +} diff --git a/cmd/sandbox/compose_test.go b/cmd/sandbox/compose_test.go new file mode 100644 index 0000000..92cb77b --- /dev/null +++ b/cmd/sandbox/compose_test.go @@ -0,0 +1,303 @@ +package sandbox + +import ( + "archive/tar" + "bytes" + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "reflect" + "strings" + "testing" + + "github.com/urfave/cli/v2" +) + +func TestResolveEgress(t *testing.T) { + t.Run("presets union and sort", func(t *testing.T) { + got, err := resolveEgress([]string{"npm", "github"}, nil) + if err != nil { + t.Fatalf("resolveEgress: %v", err) + } + want := []string{ + "codeload.github.com", "github.com", "objects.githubusercontent.com", + "raw.githubusercontent.com", "registry.npmjs.org", + } + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } + }) + + t.Run("explicit hosts merge and dedupe", func(t *testing.T) { + got, err := resolveEgress([]string{"npm"}, []string{"registry.npmjs.org", "example.com", " "}) + if err != nil { + t.Fatalf("resolveEgress: %v", err) + } + want := []string{"example.com", "registry.npmjs.org"} + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } + }) + + t.Run("nothing means unrestricted", func(t *testing.T) { + got, err := resolveEgress(nil, nil) + if err != nil { + t.Fatalf("resolveEgress: %v", err) + } + if len(got) != 0 { + t.Errorf("got %v, want empty", got) + } + }) + + t.Run("unknown preset names the real ones", func(t *testing.T) { + _, err := resolveEgress([]string{"go-modules"}, nil) + if err == nil { + t.Fatal("want an error for an unknown preset") + } + for _, name := range egressPresetNames() { + if !strings.Contains(err.Error(), name) { + t.Errorf("error does not list %q: %v", name, err) + } + } + }) +} + +func TestParseKeyValues(t *testing.T) { + got, err := parseKeyValues([]string{"A=1", "B=with=equals", "C="}) + if err != nil { + t.Fatalf("parseKeyValues: %v", err) + } + want := map[string]string{"A": "1", "B": "with=equals", "C": ""} + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } + if _, err := parseKeyValues([]string{"novalue"}); err == nil { + t.Error("want an error for a flag with no =") + } + if _, err := parseKeyValues([]string{"=1"}); err == nil { + t.Error("want an error for an empty key") + } +} + +func TestStageDirHonoursGitignore(t *testing.T) { + dir := t.TempDir() + writeFile(t, dir, ".gitignore", "node_modules/\n*.log\n") + writeFile(t, dir, "main.go", "package main\n") + writeFile(t, dir, "app.log", "noise\n") + writeFile(t, dir, filepath.Join("node_modules", "dep", "index.js"), "module.exports={}\n") + + for _, args := range [][]string{ + {"init", "-q"}, + {"config", "user.email", "t@example.com"}, + {"config", "user.name", "t"}, + } { + cmd := exec.CommandContext(t.Context(), "git", append([]string{"-C", dir}, args...)...) //#nosec G204 -- dir is t.TempDir(), args are literals in this test + if out, err := cmd.CombinedOutput(); err != nil { + t.Skipf("git unavailable: %v: %s", err, out) + } + } + + tree, err := stageDir(context.Background(), dir, stageOptions{}) + if err != nil { + t.Fatalf("stageDir: %v", err) + } + defer func() { _ = os.Remove(tree.Path) }() + + names := tarNames(t, tree.Path) + if !names["main.go"] { + t.Error("main.go missing — tracked files must ship") + } + if names["app.log"] { + t.Error("app.log shipped — .gitignore says it must not") + } + for n := range names { + if strings.HasPrefix(n, "node_modules/") { + t.Errorf("%s shipped — .gitignore says node_modules must not", n) + } + } + if names[".git/HEAD"] { + t.Error(".git shipped without IncludeGit") + } +} + +func TestStageDirWithoutGitUsesDefaultExcludes(t *testing.T) { + dir := t.TempDir() + writeFile(t, dir, "main.go", "package main\n") + writeFile(t, dir, filepath.Join("node_modules", "dep", "index.js"), "x\n") + writeFile(t, dir, filepath.Join("dist", "bundle.js"), "x\n") + writeFile(t, dir, filepath.Join("src", "keep.ts"), "x\n") + + tree, err := stageDir(context.Background(), dir, stageOptions{Exclude: []string{"src"}}) + if err != nil { + t.Fatalf("stageDir: %v", err) + } + defer func() { _ = os.Remove(tree.Path) }() + + names := tarNames(t, tree.Path) + if !names["main.go"] { + t.Error("main.go missing") + } + for _, unwanted := range []string{"node_modules/dep/index.js", "dist/bundle.js", "src/keep.ts"} { + if names[unwanted] { + t.Errorf("%s shipped — it is excluded", unwanted) + } + } +} + +func TestStageExcluded(t *testing.T) { + for _, tc := range []struct { + rel string + want bool + }{ + {"node_modules", true}, + {"node_modules/a/b.js", true}, + {"src/node_modules/x.js", true}, + {"src/main.go", false}, + {"nodes/main.go", false}, + } { + if got := stageExcluded(tc.rel, stageDefaultExcludes); got != tc.want { + t.Errorf("stageExcluded(%q) = %v, want %v", tc.rel, got, tc.want) + } + } +} + +// TestUntarIntoRefusesEscape covers the path-traversal guard. The archive +// is built inside a sandbox, so it is untrusted input on the way back out. +func TestUntarInto(t *testing.T) { + t.Run("refuses an entry above the root", func(t *testing.T) { + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + body := []byte("owned") + if err := tw.WriteHeader(&tar.Header{ + Name: "../escaped.txt", Mode: 0o600, Size: int64(len(body)), Typeflag: tar.TypeReg, + }); err != nil { + t.Fatal(err) + } + if _, err := tw.Write(body); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + + root := t.TempDir() + err := untarInto(&buf, root) + // filepath.Clean("/../escaped.txt") lands back at the root, so the + // entry must either be refused or land inside root. Never above it. + if err == nil { + if _, statErr := os.Stat(filepath.Join(filepath.Dir(root), "escaped.txt")); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("archive escaped the extraction root") + } + } + }) + + t.Run("extracts a normal tree", func(t *testing.T) { + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + body := []byte("hello") + if err := tw.WriteHeader(&tar.Header{ + Name: "coverage/report.txt", Mode: 0o644, Size: int64(len(body)), Typeflag: tar.TypeReg, + }); err != nil { + t.Fatal(err) + } + if _, err := tw.Write(body); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + + root := t.TempDir() + if err := untarInto(&buf, root); err != nil { + t.Fatalf("untarInto: %v", err) + } + got, err := os.ReadFile(filepath.Join(root, "coverage", "report.txt")) + if err != nil { + t.Fatalf("read extracted file: %v", err) + } + if string(got) != "hello" { + t.Errorf("content = %q, want %q", got, "hello") + } + }) +} + +func writeFile(t *testing.T, dir, rel, body string) { + t.Helper() + p := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +func tarNames(t *testing.T, path string) map[string]bool { + t.Helper() + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = f.Close() }() + names := map[string]bool{} + tr := tar.NewReader(f) + for { + hdr, err := tr.Next() + if err != nil { + return names + } + names[hdr.Name] = true + } +} + +// TestSplitDirAndCommand pins the `--` handling. urfave/cli passes the +// separator through as a plain argument, so a live run once shipped +// "-- ls -la" to bash and the shell failed on it. +func TestSplitDirAndCommand(t *testing.T) { + for _, tc := range []struct { + name string + args []string + wantDir string + wantCmd string + wantErr bool + }{ + {"with separator", []string{".", "--", "bun", "test"}, ".", "bun test", false}, + {"without separator", []string{".", "bun", "test"}, ".", "bun test", false}, + {"separator and a shell line", []string{"./src", "--", "echo", "a;", "echo", "b"}, "./src", "echo a; echo b", false}, + {"no command", []string{"."}, "", "", true}, + {"separator but no command", []string{".", "--"}, "", "", true}, + } { + t.Run(tc.name, func(t *testing.T) { + app := &cli.App{ + Commands: []*cli.Command{{ + Name: "offload", + Action: func(c *cli.Context) error { + dir, cmd, err := splitDirAndCommand(c) + if tc.wantErr { + if err == nil { + t.Errorf("want an error, got dir=%q cmd=%q", dir, cmd) + } + return nil + } + if err != nil { + t.Errorf("splitDirAndCommand: %v", err) + return nil + } + if dir != tc.wantDir { + t.Errorf("dir = %q, want %q", dir, tc.wantDir) + } + if cmd != tc.wantCmd { + t.Errorf("cmd = %q, want %q", cmd, tc.wantCmd) + } + return nil + }, + }}, + } + if err := app.Run(append([]string{"createos", "offload"}, tc.args...)); err != nil { + t.Fatalf("app.Run: %v", err) + } + }) + } +} diff --git a/cmd/sandbox/create.go b/cmd/sandbox/create.go index b4c317c..8beb158 100644 --- a/cmd/sandbox/create.go +++ b/cmd/sandbox/create.go @@ -312,6 +312,7 @@ func printCreateResult(resp *api.SandboxCreateResp) { pterm.Success.Println("Reachable from anywhere over HTTPS:") fmt.Printf(" %s\n", resp.IngressURLTemplate) pterm.Println(pterm.Gray(" Replace with the port your service is listening on.")) + warnIngressCaveats() } if resp.AutoPauseAfterSeconds != nil { diff --git a/cmd/sandbox/edit.go b/cmd/sandbox/edit.go index 203aac9..a6983fb 100644 --- a/cmd/sandbox/edit.go +++ b/cmd/sandbox/edit.go @@ -328,6 +328,7 @@ func applyIngressFlag(c *cli.Context, client *api.SandboxClient, label, id, valu fmt.Printf(" %s\n", updated.IngressURLTemplate) pterm.Println(pterm.Gray(" Replace with the port your service is listening on.")) } + warnIngressCaveats() } else { pterm.Success.Printfln("Public URL is off for %s", refLabel(label, id)) } diff --git a/cmd/sandbox/egress_preset.go b/cmd/sandbox/egress_preset.go new file mode 100644 index 0000000..d195242 --- /dev/null +++ b/cmd/sandbox/egress_preset.go @@ -0,0 +1,76 @@ +package sandbox + +import ( + "fmt" + "sort" + "strings" +) + +// egressPresets name the hosts one toolchain needs to fetch its +// dependencies. A sandbox with no --egress reaches the whole internet; +// naming a preset is the cheap way to close that down to the registry the +// build actually uses, without anyone having to remember that cargo also +// pulls from static.rust-lang.org. +// +// Presets compose: --egress-preset npm --egress-preset github unions both +// lists, and --egress adds single hosts on top. +var egressPresets = map[string][]string{ + "python-uv": { + "astral.sh", "releases.astral.sh", "pypi.org", "files.pythonhosted.org", + }, + "rust-cargo": { + "crates.io", "static.crates.io", "index.crates.io", "static.rust-lang.org", "cdn.pyke.io", + }, + "npm": { + "registry.npmjs.org", + }, + "github": { + "github.com", "objects.githubusercontent.com", "raw.githubusercontent.com", "codeload.github.com", + }, +} + +// egressPresetNames lists the presets in a stable order, for help text +// and error messages. +func egressPresetNames() []string { + names := make([]string, 0, len(egressPresets)) + for n := range egressPresets { + names = append(names, n) + } + sort.Strings(names) + return names +} + +// resolveEgress expands preset names and merges them with explicit hosts. +// The result is deduplicated and sorted, so two invocations with the same +// intent produce the same allowlist. +// +// An empty result means "unrestricted", which is what the backend does +// with an empty list. Callers that want to warn about that check len(). +func resolveEgress(presets, hosts []string) ([]string, error) { + seen := make(map[string]struct{}) + for _, p := range presets { + p = strings.TrimSpace(p) + if p == "" { + continue + } + domains, ok := egressPresets[p] + if !ok { + return nil, fmt.Errorf("unknown egress preset %q\n\n Available: %s", + p, strings.Join(egressPresetNames(), ", ")) + } + for _, d := range domains { + seen[d] = struct{}{} + } + } + for _, h := range hosts { + if h = strings.TrimSpace(h); h != "" { + seen[h] = struct{}{} + } + } + out := make([]string, 0, len(seen)) + for h := range seen { + out = append(out, h) + } + sort.Strings(out) + return out, nil +} diff --git a/cmd/sandbox/fork.go b/cmd/sandbox/fork.go index bd55de9..04f7f6d 100644 --- a/cmd/sandbox/fork.go +++ b/cmd/sandbox/fork.go @@ -1,6 +1,7 @@ package sandbox import ( + "context" "fmt" "strings" @@ -21,12 +22,31 @@ func newForkCommand() *cli.Command { default the fork auto-resumes; pass --paused to keep it paused so you can fork again or attach things first. -Run with no argument on a terminal to pick from your paused sandboxes.`, +Pass --count to take several clones of one prepared sandbox — a golden +box with the toolchain and dependencies already installed, cloned once +per test job or per user. Each clone is independent. + +Run with no argument on a terminal to pick from your paused sandboxes. + +Examples: + # One clone, resumed and ready + createos sandbox fork my-golden-box + + # Ten independent clones, left paused so you resume them when needed + createos sandbox fork my-golden-box --count 10 --paused + +A forked sandbox comes up WITHOUT the S3 disks the source had mounted. +Re-attach them after the fork resumes.`, Flags: []cli.Flag{ &cli.BoolFlag{ Name: "paused", Usage: "Leave the new sandbox paused instead of auto-resuming", }, + &cli.IntFlag{ + Name: "count", + Value: 1, + Usage: "Number of clones to take from the same snapshot", + }, &cli.StringSliceFlag{ Name: "ssh-key", Usage: "Override SSH public-key file for the fork (repeatable)", @@ -69,6 +89,11 @@ func runFork(c *cli.Context) error { } func runForkByID(c *cli.Context, client *api.SandboxClient, ref, srcID string) error { + count := c.Int("count") + if count < 1 { + return fmt.Errorf("--count must be at least 1 (got %d)", count) + } + req := api.SandboxForkReq{ StartPaused: c.Bool("paused"), } @@ -82,53 +107,195 @@ func runForkByID(c *cli.Context, client *api.SandboxClient, ref, srcID string) e } if output.IsJSON(c) { - view, err := client.ForkSandbox(c.Context, srcID, req) + forks, err := forkN(c.Context, client, srcID, req, count, nil) if err != nil { return err } - target := "running" - if req.StartPaused { - target = "paused" - } - sb, err := waitForStatus(c.Context, client, view.ID, target) - if err != nil { - return err + if count == 1 { + output.Render(c, forks[0], func() {}) + return nil } - output.Render(c, sb, func() {}) + output.Render(c, forks, func() {}) return nil } - spinner, _ := pterm.DefaultSpinner.Start(fmt.Sprintf("Forking %s…", refLabel(ref, srcID))) //nolint:errcheck - view, err := client.ForkSandbox(c.Context, srcID, req) + warnForkDropsDisks(c.Context, client, srcID) + + label := refLabel(ref, srcID) + noun := "Forking %s…" + if count > 1 { + noun = fmt.Sprintf("Forking %%s into %d clones…", count) + } + spinner, _ := pterm.DefaultSpinner.Start(fmt.Sprintf(noun, label)) //nolint:errcheck + forks, err := forkN(c.Context, client, srcID, req, count, func(done, total int) { + if total > 1 { + spinner.UpdateText(fmt.Sprintf("Forking %s — %d/%d ready…", label, done, total)) + } + }) if err != nil { spinner.Fail("Fork failed") return err } - target := "running" - if req.StartPaused { - target = "paused" + spinner.Success(fmt.Sprintf("Forked %s into %d sandbox(es)", label, len(forks))) + for _, sb := range forks { + name := "" + if sb.Name != nil { + name = *sb.Name + } + fmt.Printf(" %s\n", refLabel(name, sb.ID)) + if sb.IP != nil && *sb.IP != "" { + fmt.Printf(" IP: %s\n", *sb.IP) + } + if sb.IngressURLTemplate != "" { + fmt.Printf(" URL: %s\n", sb.IngressURLTemplate) + } } - sb, err := waitForStatus(c.Context, client, view.ID, target) + return nil +} + +// ensureForkable brings srcID to `paused`, which is the only state fork +// accepts. Pause is asynchronous: it answers while the sandbox is still +// `pausing`, so a fork issued right after a pause used to be rejected with +// "sandbox is running, expected paused or error". Waiting here is what +// makes pause-then-fork safe for callers, matrix included. +func ensureForkable(ctx context.Context, client *api.SandboxClient, srcID string) error { + sb, err := client.GetSandbox(ctx, srcID) if err != nil { - spinner.Fail("Fork did not finish") return err } - if sb.Status != target { - spinner.Fail(fmt.Sprintf("Fork ended in %q", sb.Status)) - return fmt.Errorf("sandbox %s is %s — see `createos sandbox get %s` for details", sb.ID, sb.Status, sb.ID) + switch sb.Status { + case "paused": + return nil + case "pausing": + // Already on its way down; waiting is not a decision we are making + // on the user's behalf. + return waitUntilPaused(ctx, client, srcID) + case "running": + // Deliberately NOT pausing here. Pausing a running sandbox stops + // whatever it is serving, and `fork` must never do that as a side + // effect — the source could be a live dev server or a demo someone + // is using. Callers that own the sandbox pause it themselves. + return fmt.Errorf( + "sandbox %s is running, and fork needs a paused snapshot\n\n Pausing stops whatever it is serving, so fork will not do it for you.\n Pause it yourself, then fork:\n createos sandbox pause %s\n createos sandbox fork %s", + srcID, srcID, srcID) + default: + return fmt.Errorf("sandbox %s is %s — fork needs it paused\n\n Run:\n createos sandbox pause %s", srcID, sb.Status, srcID) } +} - name := "" - if sb.Name != nil { - name = *sb.Name +// pauseForFork pauses a sandbox the caller owns and waits for the snapshot +// to settle. Only matrix uses this, on the golden box it created itself — +// which is the one case where pausing is not a surprise to anyone. +func pauseForFork(ctx context.Context, client *api.SandboxClient, srcID string) error { + sb, err := client.GetSandbox(ctx, srcID) + if err != nil { + return err + } + switch sb.Status { + case "paused": + return nil + case "pausing": + case "running": + if _, pauseErr := client.PauseSandbox(ctx, srcID); pauseErr != nil { + return fmt.Errorf("pause %s before forking: %w", srcID, pauseErr) + } + default: + return fmt.Errorf("sandbox %s is %s — it cannot be paused for forking", srcID, sb.Status) } - spinner.Success(fmt.Sprintf("Forked into %s", refLabel(name, sb.ID))) - if sb.IP != nil && *sb.IP != "" { - fmt.Printf(" IP: %s\n", *sb.IP) + return waitUntilPaused(ctx, client, srcID) +} + +// waitUntilPaused blocks until the snapshot is on disk. Pause is async: it +// answers while the sandbox is still `pausing`, and a fork issued in that +// window is rejected with "sandbox is running, expected paused or error". +func waitUntilPaused(ctx context.Context, client *api.SandboxClient, srcID string) error { + final, err := waitForStatus(ctx, client, srcID, "paused") + if err != nil { + return err } - if sb.IngressURLTemplate != "" { - fmt.Printf(" URL: %s\n", sb.IngressURLTemplate) + if final.Status != "paused" { + return fmt.Errorf("sandbox %s ended in %q while pausing — see `createos sandbox get %s`", srcID, final.Status, srcID) } return nil } + +// forkN takes count clones of one paused snapshot and waits for each to +// reach its target state. onProgress, when non-nil, is called after every +// clone settles. +// +// Clones run one at a time on purpose. Fork is a server-side object copy +// measured at about a second, so the wall-clock saving from parallelism is +// small, while a partial failure halfway through a parallel batch leaves +// an unknown number of billable sandboxes behind. Sequential means the +// error names exactly how many exist. +func forkN( + ctx context.Context, + client *api.SandboxClient, + srcID string, + req api.SandboxForkReq, + count int, + onProgress func(done, total int), +) ([]*api.SandboxView, error) { + if err := ensureForkable(ctx, client, srcID); err != nil { + return nil, err + } + target := "running" + if req.StartPaused { + target = "paused" + } + + // created tracks every id the server handed back, settled or not. + // forks holds only the ones that reached `target`. The split matters: + // a fork whose status poll times out still exists and still bills, and + // reporting only the settled ones hides it from the caller — which, + // for matrix, is the difference between a cleaned-up failure and an + // orphaned running sandbox. + forks := make([]*api.SandboxView, 0, count) + created := make([]string, 0, count) + for i := 0; i < count; i++ { + view, err := client.ForkSandbox(ctx, srcID, req) + if err != nil { + return forks, forkPartialError(err, created, i, count) + } + created = append(created, view.ID) + + sb, err := waitForStatus(ctx, client, view.ID, target) + if err != nil { + return forks, forkPartialError(err, created, i, count) + } + if sb.Status != target { + return forks, forkPartialError( + fmt.Errorf("sandbox %s is %s, expected %s", sb.ID, sb.Status, target), created, i, count) + } + forks = append(forks, sb) + if onProgress != nil { + onProgress(len(forks), count) + } + } + return forks, nil +} + +// forkPartialError names every clone the server created, so a failed batch +// does not leave billable sandboxes the caller cannot find. It carries the +// ids as a forkLeak so a caller that can clean up — matrix — does not have +// to parse them back out of the message. +func forkPartialError(err error, created []string, attempt, total int) error { + if len(created) == 0 { + return err + } + return &forkLeak{ + IDs: created, + err: fmt.Errorf("fork %d of %d failed: %w\n\n %d clone(s) exist and are still billable:\n %s\n\n Remove them with:\n createos sandbox rm --force %s", + attempt+1, total, err, len(created), strings.Join(created, "\n "), strings.Join(created, " ")), + } +} + +// forkLeak reports the sandboxes a failed fork batch left behind. +type forkLeak struct { + IDs []string + err error +} + +func (e *forkLeak) Error() string { return e.err.Error() } +func (e *forkLeak) Unwrap() error { return e.err } diff --git a/cmd/sandbox/guards.go b/cmd/sandbox/guards.go new file mode 100644 index 0000000..fd960a8 --- /dev/null +++ b/cmd/sandbox/guards.go @@ -0,0 +1,91 @@ +package sandbox + +import ( + "context" + "fmt" + "path" + "strings" + + "github.com/pterm/pterm" + + "github.com/NodeOps-app/createos-cli/internal/api" +) + +// Guards for platform behaviour that surprises people. Each one traces to +// a tracked issue, and each one exists because the failure it prevents is +// silent: data that never lands, a clone that runs against an empty +// directory, a preview URL that a browser refuses. A warning at the moment +// of the action costs far less than the debugging session it replaces. + +// diskMountBlocksFileAPI reports the mount path that would swallow remote, +// or "" when the path is safe to move through the file API. +// +// Writing into an S3 disk mount through the file API crashes the mount and +// loses the object (issue #71). The write looks like it worked, so nothing +// tells the user until the data is missing. Reading is equally unsafe, so +// both push and pull consult this. +// +// It fails CLOSED. If the disk list cannot be read, the mount state is +// unknown, and "unknown" is not "safe": carrying on risks destroying data +// the user believes they just saved, while refusing costs them a retry. +// The two outcomes are not comparable, so the unknown case refuses. +func diskMountBlocksFileAPI(ctx context.Context, client *api.SandboxClient, sandboxID, remote string) (string, error) { + disks, err := client.ListSandboxDisks(ctx, sandboxID) + if err != nil { + return "", fmt.Errorf( + "could not check whether %s is inside an S3 disk mount on %s: %w\n\n Moving a file into a disk mount through the file API crashes the mount\n and loses the object (issue #71), so this stops rather than risk it.\n Retry, or copy from inside the sandbox:\n createos sandbox exec %s -- bash -lc 'cp ...'", + remote, sandboxID, err, sandboxID) + } + clean := path.Clean(remote) + for _, d := range disks { + mount := path.Clean(strings.TrimSpace(d.MountPath)) + if mount == "" || mount == "." || mount == "/" { + continue + } + if clean == mount || strings.HasPrefix(clean, mount+"/") { + return mount, nil + } + } + return "", nil +} + +// diskMountFileAPIError is the refusal. This is a hard stop rather than a +// warning: the documented outcome is a crashed mount and a lost object, so +// carrying on would destroy data the user believes they just saved. +func diskMountFileAPIError(remote, mount, verb string) error { + inner := "cp /local/file " + remote + if verb == "pull" { + inner = "cp " + remote + " /tmp/copy" + } + return fmt.Errorf( + "%s is inside the S3 disk mounted at %s, and the file API cannot move data through a disk mount (issue #71)\n\n The transfer would crash the mount and lose the object.\n Do it from inside the sandbox instead:\n createos sandbox exec -- bash -lc '%s'", + remote, mount, inner) +} + +// warnForkDropsDisks says what a fork will silently not carry. +// +// A forked sandbox comes up without its source's disk attachments (issue +// #63), so a job on the clone reads an empty directory and can "pass" +// against nothing at all. +func warnForkDropsDisks(ctx context.Context, client *api.SandboxClient, srcID string) { + disks, err := client.ListSandboxDisks(ctx, srcID) + if err != nil || len(disks) == 0 { + return + } + mounts := make([]string, 0, len(disks)) + for _, d := range disks { + mounts = append(mounts, d.Name+" at "+d.MountPath) + } + pterm.Warning.Printfln( + "The fork will come up WITHOUT the %d disk(s) this sandbox has mounted (issue #63):\n %s\n Re-attach them after the fork resumes, or it will read empty directories.", + len(disks), strings.Join(mounts, "\n ")) +} + +// warnIngressCaveats fires when a public HTTPS URL is switched on. Both +// caveats cost real debugging time and neither is visible from the URL. +func warnIngressCaveats() { + pterm.Warning.Println("The public URL has two known limits:") + fmt.Println(" TLS is a self-signed certificate, so browsers reject it (issue #46).") + fmt.Println(" The ingress hop strips the Authorization header, so services gated") + fmt.Println(" on Basic or Bearer auth see no credentials (issue #64).") +} diff --git a/cmd/sandbox/guards_test.go b/cmd/sandbox/guards_test.go new file mode 100644 index 0000000..e938c73 --- /dev/null +++ b/cmd/sandbox/guards_test.go @@ -0,0 +1,125 @@ +package sandbox + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestDiskMountBlocksFileAPI covers the guard for issue #71: a file-API +// transfer into an S3 disk mount crashes the mount and loses the object. +// The path comparison has to be on whole segments — "/mnt/data-old" is not +// inside "/mnt/data", and blocking it would stop a legitimate transfer. +func TestDiskMountBlocksFileAPI(t *testing.T) { + f := newFakeAPI(t).json("GET /v1/sandboxes/sb-1/disks", + `{"data":{"data":[{"disk_id":"d1","name":"bucket","mount_path":"/mnt/data"}]}}`) + + for _, tc := range []struct { + remote string + want string + }{ + {"/mnt/data/report.csv", "/mnt/data"}, + {"/mnt/data", "/mnt/data"}, + {"/mnt/data/nested/deep.bin", "/mnt/data"}, + {"/workspace/report.csv", ""}, + {"/mnt/data-old/report.csv", ""}, + {"/mnt", ""}, + } { + got, err := diskMountBlocksFileAPI(context.Background(), f.client(), "sb-1", tc.remote) + if err != nil { + t.Fatalf("diskMountBlocksFileAPI(%q): %v", tc.remote, err) + } + if got != tc.want { + t.Errorf("diskMountBlocksFileAPI(%q) = %q, want %q", tc.remote, got, tc.want) + } + } +} + +// A sandbox with no disks must never be blocked. +func TestDiskMountAllowsASandboxWithNoDisks(t *testing.T) { + empty := newFakeAPI(t).json("GET /v1/sandboxes/sb-1/disks", `{"data":{"data":[]}}`) + got, err := diskMountBlocksFileAPI(context.Background(), empty.client(), "sb-1", "/anything") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "" { + t.Errorf("no disks attached, got %q, want no block", got) + } +} + +// TestDiskMountFailsClosed is the important half. When the disk list +// cannot be read the mount state is unknown, and "unknown" must not be +// treated as "safe": a wrong guess here crashes an S3 mount and loses the +// object, while a refusal only costs a retry. +func TestDiskMountFailsClosed(t *testing.T) { + broken := newFakeAPI(t).fails("GET /v1/sandboxes/sb-1/disks") + _, err := diskMountBlocksFileAPI(context.Background(), broken.client(), "sb-1", "/workspace/out.csv") + if err == nil { + t.Fatal("disk list unreadable but the transfer was allowed — this is how the object gets lost") + } + for _, want := range []string{"/workspace/out.csv", "#71", "sandbox exec"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must mention %q, got: %v", want, err) + } + } +} + +func TestDiskMountFileAPIError(t *testing.T) { + err := diskMountFileAPIError("/mnt/data/out.csv", "/mnt/data", "push") + for _, want := range []string{"/mnt/data/out.csv", "/mnt/data", "#71", "sandbox exec"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must mention %q, got: %v", want, err) + } + } +} + +// TestSelfSignalHTTP checks the wire shape the guest agent expects: a POST +// to /self/, the reason carried as a query parameter, and 202 +// treated as success. +func TestSelfSignalHTTP(t *testing.T) { + var gotMethod, gotPath, gotReason string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod, gotPath, gotReason = r.Method, r.URL.Path, r.URL.Query().Get("reason") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"status":"accepted","action":"pause"}`)) + })) + defer srv.Close() + + // selfSignalHTTP hardcodes the loopback address, so point the test at + // the fake server by rebuilding the same request shape it sends. + addr := strings.TrimPrefix(srv.URL, "http://") + original := selfSignalAddrForTest + selfSignalAddrForTest = addr + t.Cleanup(func() { selfSignalAddrForTest = original }) + + if err := selfSignalHTTP(context.Background(), "pause", "job done"); err != nil { + t.Fatalf("selfSignalHTTP: %v", err) + } + if gotMethod != http.MethodPost { + t.Errorf("method = %s, want POST", gotMethod) + } + if gotPath != "/self/pause" { + t.Errorf("path = %s, want /self/pause", gotPath) + } + if gotReason != "job done" { + t.Errorf("reason = %q, want %q", gotReason, "job done") + } +} + +func TestSelfSignalHTTPRejectsUnexpectedStatus(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + })) + defer srv.Close() + + original := selfSignalAddrForTest + selfSignalAddrForTest = strings.TrimPrefix(srv.URL, "http://") + t.Cleanup(func() { selfSignalAddrForTest = original }) + + err := selfSignalHTTP(context.Background(), "pause", "") + if err == nil { + t.Fatal("want an error when something other than the agent answers") + } +} diff --git a/cmd/sandbox/lifecycle_test.go b/cmd/sandbox/lifecycle_test.go new file mode 100644 index 0000000..a90d698 --- /dev/null +++ b/cmd/sandbox/lifecycle_test.go @@ -0,0 +1,342 @@ +package sandbox + +import ( + "archive/tar" + "bytes" + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/urfave/cli/v2" + + "github.com/NodeOps-app/createos-cli/internal/api" +) + +// These tests cover the failure paths, not the happy ones. Every one of +// them exists because a success path that leaks a billable sandbox looks +// exactly like a success path that does not. + +// fakeAPI is a stand-in sandbox control plane. Handlers are matched by +// "METHOD /path" with {id} already substituted, so a test only declares +// the calls it cares about; anything else is a 404 the test can assert on. +type fakeAPI struct { + t *testing.T + mu sync.Mutex + seen []string + handlers map[string]http.HandlerFunc + srv *httptest.Server +} + +func newFakeAPI(t *testing.T) *fakeAPI { + t.Helper() + f := &fakeAPI{t: t, handlers: map[string]http.HandlerFunc{}} + f.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + key := r.Method + " " + r.URL.Path + f.mu.Lock() + f.seen = append(f.seen, key) + h, ok := f.handlers[key] + f.mu.Unlock() + if !ok { + http.Error(w, `{"error":"no handler: `+key+`"}`, http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + h(w, r) + })) + t.Cleanup(f.srv.Close) + return f +} + +func (f *fakeAPI) on(key string, h http.HandlerFunc) *fakeAPI { + f.mu.Lock() + defer f.mu.Unlock() + f.handlers[key] = h + return f +} + +func (f *fakeAPI) json(key, body string) *fakeAPI { + return f.on(key, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(body)) + }) +} + +// fails makes an endpoint answer 500. Every caller wants the same thing — +// "this control-plane call is broken right now" — so the status is fixed +// rather than a parameter nobody varies. +func (f *fakeAPI) fails(key string) *fakeAPI { + return f.on(key, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"boom"}`)) + }) +} + +func (f *fakeAPI) called(key string) bool { + f.mu.Lock() + defer f.mu.Unlock() + for _, s := range f.seen { + if s == key { + return true + } + } + return false +} + +func (f *fakeAPI) client() *api.SandboxClient { + c := api.NewSandboxClient("tok", f.srv.URL, false) + return &c +} + +// shortPoll makes waitForStatus give up quickly. Without it these tests +// would sit through the production 5-minute timeout. +func shortPoll(t *testing.T) context.Context { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + t.Cleanup(cancel) + return ctx +} + +// TestCreateComposeBoxDestroysWhenNeverReady covers the leak Codex found: +// CreateSandbox succeeds, readiness polling does not, and the caller only +// ever sees an error — so if createComposeBox does not destroy the box +// itself, nothing will. +func TestCreateComposeBoxDestroysWhenNeverReady(t *testing.T) { + f := newFakeAPI(t). + json("POST /v1/sandboxes", `{"data":{"id":"sb-stuck"}}`). + json("GET /v1/sandboxes/sb-stuck", `{"data":{"id":"sb-stuck","status":"failed"}}`). + json("DELETE /v1/sandboxes/sb-stuck", `{"data":{"id":"sb-stuck","status":"destroying"}}`) + + _, err := createComposeBox(shortPoll(t), f.client(), &composeOptions{Shape: "s-1vcpu-1gb"}) + if err == nil { + t.Fatal("want an error when the sandbox never reaches running") + } + if !f.called("DELETE /v1/sandboxes/sb-stuck") { + t.Error("sandbox was created and never destroyed — it is still billable") + } + if !strings.Contains(err.Error(), "sb-stuck") { + t.Errorf("error must name the sandbox id, got: %v", err) + } +} + +// TestCreateComposeBoxReportsUndestroyableBox is the worse branch: the box +// exists and teardown also failed, so the id must reach the user with a +// command they can run by hand. +func TestCreateComposeBoxReportsUndestroyableBox(t *testing.T) { + f := newFakeAPI(t). + json("POST /v1/sandboxes", `{"data":{"id":"sb-orphan"}}`). + json("GET /v1/sandboxes/sb-orphan", `{"data":{"id":"sb-orphan","status":"failed"}}`). + fails("DELETE /v1/sandboxes/sb-orphan") + + _, err := createComposeBox(shortPoll(t), f.client(), &composeOptions{Shape: "s-1vcpu-1gb"}) + if err == nil { + t.Fatal("want an error") + } + for _, want := range []string{"sb-orphan", "still billable", "rm --force"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must contain %q so the user can clean up, got: %v", want, err) + } + } +} + +// TestForkNReportsCloneCreatedBeforePollFailed covers the orphan Codex +// found: ForkSandbox returns a real, running clone, then the status poll +// fails. The clone is not in the settled list, so unless the error carries +// its id, nothing can ever clean it up. +func TestForkNReportsCloneCreatedBeforePollFailed(t *testing.T) { + f := newFakeAPI(t). + json("GET /v1/sandboxes/sb-golden", `{"data":{"id":"sb-golden","status":"paused"}}`). + json("POST /v1/sandboxes/sb-golden/fork", `{"data":{"id":"sb-clone-1","status":"forking"}}`). + json("GET /v1/sandboxes/sb-clone-1", `{"data":{"id":"sb-clone-1","status":"failed"}}`) + + forks, err := forkN(shortPoll(t), f.client(), "sb-golden", api.SandboxForkReq{}, 1, nil) + if err == nil { + t.Fatal("want an error when the clone never settles") + } + if len(forks) != 0 { + t.Errorf("settled forks = %d, want 0", len(forks)) + } + + var leak *forkLeak + if !errors.As(err, &leak) { + t.Fatalf("error must be a *forkLeak carrying the created id, got %T: %v", err, err) + } + if len(leak.IDs) != 1 || leak.IDs[0] != "sb-clone-1" { + t.Errorf("leak.IDs = %v, want [sb-clone-1]", leak.IDs) + } + if !strings.Contains(err.Error(), "sb-clone-1") { + t.Errorf("message must name the clone, got: %v", err) + } +} + +// TestMatrixRunOneSurfacesDestroyFailure pins the named-return fix. The +// teardown runs in a defer; with an unnamed return Go copies the result +// before defers run, so a failed destroy never reached the caller and the +// matrix exited 0 while a clone kept billing. +func TestMatrixRunOneSurfacesDestroyFailure(t *testing.T) { + f := newFakeAPI(t). + json("GET /v1/sandboxes/sb-golden", `{"data":{"id":"sb-golden","status":"paused"}}`). + json("POST /v1/sandboxes/sb-golden/fork", `{"data":{"id":"sb-clone","status":"running"}}`). + json("GET /v1/sandboxes/sb-clone", `{"data":{"id":"sb-clone","status":"running"}}`). + json("PATCH /v1/sandboxes/sb-clone", `{"data":{"id":"sb-clone","status":"running"}}`). + json("POST /v1/sandboxes/sb-clone/processes", `{"data":{"process_id":"p1","state":"running"}}`). + on("GET /v1/sandboxes/sb-clone/processes/p1/connect", func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintln(w, `{"type":"exit","exit_code":0}`) + }). + fails("DELETE /v1/sandboxes/sb-clone") + + res := matrixRunOne(shortPoll(t), f.client(), "sb-golden", 0, "true", t.TempDir(), + &composeOptions{Shape: "s-1vcpu-1gb", AutoPause: time.Minute}) + + if res.ExitCode != 0 { + t.Fatalf("job exit code = %d, want 0 — the command itself passed", res.ExitCode) + } + if res.Error == "" { + t.Fatal("destroy failed but the job reported no error — matrix would exit 0 having leaked sb-clone") + } + if !strings.Contains(res.Error, "sb-clone") { + t.Errorf("error must name the leaked clone, got: %q", res.Error) + } +} + +// TestUntarIntoRefusesSymlinkAncestor is the extraction-boundary +// regression. A lexical prefix check passes here, because the pathname +// this code builds stays under root; the escape happens when the +// filesystem follows a symlink that was already on disk. +func TestUntarIntoRefusesSymlinkAncestor(t *testing.T) { + base := t.TempDir() + root := filepath.Join(base, "repo") + outside := filepath.Join(base, "outside") + for _, d := range []string{root, outside} { + if err := os.MkdirAll(d, 0o750); err != nil { + t.Fatal(err) + } + } + victim := filepath.Join(outside, "report.txt") + if err := os.WriteFile(victim, []byte("original"), 0o600); err != nil { + t.Fatal(err) + } + // The trap: a symlink that already exists inside the extraction root. + if err := os.Symlink(outside, filepath.Join(root, "coverage")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + body := []byte("owned") + if err := tw.WriteHeader(&tar.Header{ + Name: "coverage/report.txt", Mode: 0o644, Size: int64(len(body)), Typeflag: tar.TypeReg, + }); err != nil { + t.Fatal(err) + } + if _, err := tw.Write(body); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + + err := untarInto(&buf, root) + + got, readErr := os.ReadFile(victim) // #nosec G304 -- path built from t.TempDir() + if readErr != nil { + t.Fatalf("read victim: %v", readErr) + } + if string(got) != "original" { + t.Fatalf("archive wrote through the symlink and overwrote %s (untarInto err=%v)", victim, err) + } + if err == nil { + t.Error("want an error for an entry whose parent escapes the root") + } +} + +// TestForkRefusesToPauseARunningSandbox is the guard against the worst +// version of this command: `createos sandbox fork my-live-server` pausing +// a service somebody is using, as a side effect of asking for a clone. +// Fork must never stop a running workload on its own. +func TestForkRefusesToPauseARunningSandbox(t *testing.T) { + f := newFakeAPI(t). + json("GET /v1/sandboxes/sb-live", `{"data":{"id":"sb-live","status":"running"}}`) + + err := ensureForkable(shortPoll(t), f.client(), "sb-live") + if err == nil { + t.Fatal("want a refusal — forking must not pause a running sandbox") + } + if f.called("POST /v1/sandboxes/sb-live/pause") { + t.Error("fork paused a running sandbox on its own; whatever it was serving just stopped") + } + for _, want := range []string{"running", "createos sandbox pause sb-live"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must contain %q so the user can act, got: %v", want, err) + } + } +} + +// pauseForFork is the opposite case: matrix built the golden sandbox, so +// it is allowed to pause it. +func TestPauseForForkPausesASandboxTheCallerOwns(t *testing.T) { + pauses := 0 + f := newFakeAPI(t). + on("GET /v1/sandboxes/sb-golden", func(w http.ResponseWriter, _ *http.Request) { + status := "running" + if pauses > 0 { + status = "paused" + } + _, _ = fmt.Fprintf(w, `{"data":{"id":"sb-golden","status":%q}}`, status) + }). + on("POST /v1/sandboxes/sb-golden/pause", func(w http.ResponseWriter, _ *http.Request) { + pauses++ + _, _ = w.Write([]byte(`{"data":{"id":"sb-golden","status":"pausing"}}`)) + }) + + if err := pauseForFork(shortPoll(t), f.client(), "sb-golden"); err != nil { + t.Fatalf("pauseForFork: %v", err) + } + if pauses != 1 { + t.Errorf("pause called %d times, want 1", pauses) + } +} + +// TestOffloadFailsWhenTeardownFails covers the leak that looks like a +// clean run: the workload passes, DestroySandbox fails, and a CI job +// reading only the exit status would never learn that a billable sandbox +// was left behind. `offload` promises a throwaway sandbox, so a teardown +// failure has to reach the exit code. +func TestOffloadFailsWhenTeardownFails(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main\n"), 0o600); err != nil { + t.Fatal(err) + } + + f := newFakeAPI(t). + json("POST /v1/sandboxes", `{"data":{"id":"sb-off"}}`). + json("GET /v1/sandboxes/sb-off", `{"data":{"id":"sb-off","status":"running"}}`). + json("PUT /v1/sandboxes/sb-off/files", `{"data":{}}`). + json("POST /v1/sandboxes/sb-off/exec", `{"data":{"result":{"exit_code":0}}}`). + json("POST /v1/sandboxes/sb-off/processes", `{"data":{"process_id":"p1","state":"running"}}`). + on("GET /v1/sandboxes/sb-off/processes/p1/connect", func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintln(w, `{"type":"exit","exit_code":0}`) + }). + fails("DELETE /v1/sandboxes/sb-off") + + app := &cli.App{ + Commands: []*cli.Command{newOffloadCommand()}, + Metadata: map[string]any{api.SandboxClientKey: f.client()}, + } + err := app.RunContext(shortPoll(t), []string{"createos", "offload", dir, "--", "true"}) + + if err == nil { + t.Fatal("workload passed but the sandbox leaked, and offload reported success") + } + for _, want := range []string{"sb-off", "not destroyed", "rm --force"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error must contain %q, got: %v", want, err) + } + } +} diff --git a/cmd/sandbox/matrix.go b/cmd/sandbox/matrix.go new file mode 100644 index 0000000..deb3ffc --- /dev/null +++ b/cmd/sandbox/matrix.go @@ -0,0 +1,474 @@ +package sandbox + +import ( + "bufio" + "bytes" + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "time" + + "github.com/pterm/pterm" + "github.com/urfave/cli/v2" + + "github.com/NodeOps-app/createos-cli/internal/api" + "github.com/NodeOps-app/createos-cli/internal/output" +) + +// matrixDefaultConcurrency matches the sandboxes one account may run at +// once. Raising it past the account limit does not go faster; it just +// fails later. +const matrixDefaultConcurrency = 10 + +func newMatrixCommand() *cli.Command { + return &cli.Command{ + Name: "matrix", + Usage: "Run many commands in parallel, each on its own clone of one prepared sandbox", + ArgsUsage: "", + Description: `Matrix runs a set of commands at the same time, each on its own sandbox, +and gives you one exit code per command. + +The sandboxes are clones, not fresh machines. Matrix builds one golden +sandbox, runs --prepare on it once, pauses it, and forks that snapshot per +job. A dependency install that takes two minutes is paid once, not once per +job. Fork is a snapshot copy, so a clone costs about a second. + +Every sandbox is destroyed when its job finishes. The command exits 0 only +if every job exited 0. + +Flags must come before the directory. Anything after it is not read as a +flag. + +Examples: + # Three test suites, three sandboxes, one dependency install + createos sandbox matrix --prepare 'bun install' \ + --job 'bun test unit' --job 'bun test e2e' --job 'bun test perf' . + + # A large matrix from a file, ten at a time + createos sandbox matrix --prepare 'npm ci' --jobs-file cases.txt --concurrency 10 . + + # Reuse a sandbox you already prepared and paused + createos sandbox matrix --from my-golden-box --job 'pytest -k slow' + +Known limits: + A fork comes up without the S3 disks its source had mounted (issue #63), + so matrix refuses --disk rather than run jobs against missing data. + A clone whose snapshot is not cached on the target host takes 11-13 + seconds to resume, not under a second. + A clone does not inherit --shape from the golden sandbox, so clones can + be larger than you asked for and cost more. Matrix does re-apply + --auto-pause to every clone, because a clone does not inherit that + either.`, + Flags: append(composeFlags(), + &cli.StringFlag{ + Name: "prepare", + Usage: "Command to run once on the golden sandbox before it is cloned", + }, + &cli.StringSliceFlag{ + Name: "job", + Usage: "Command to run on its own clone (repeatable)", + }, + &cli.StringFlag{ + Name: "jobs-file", + Usage: "File with one job command per line. Blank lines and lines starting with # are skipped", + }, + &cli.IntFlag{ + Name: "concurrency", + Value: matrixDefaultConcurrency, + Usage: "How many clones run at the same time", + }, + &cli.StringFlag{ + Name: "from", + Usage: "Clone this existing sandbox instead of building a golden one from ", + }, + &cli.StringFlag{ + Name: "logs", + Usage: "Directory for per-job log files. Default: a temporary directory", + }, + &cli.BoolFlag{ + Name: "keep-golden", + Usage: "Keep the golden sandbox after the run instead of destroying it", + }, + ), + Action: runMatrix, + } +} + +// matrixJobResult is one job's outcome, and one row of the JSON output. +type matrixJobResult struct { + Index int `json:"index"` + Cmd string `json:"cmd"` + Sandbox string `json:"sandbox,omitempty"` + ExitCode int `json:"exit_code"` + DurationMs int64 `json:"duration_ms"` + Log string `json:"log,omitempty"` + Error string `json:"error,omitempty"` +} + +func runMatrix(c *cli.Context) error { + client, ok := c.App.Metadata[api.SandboxClientKey].(*api.SandboxClient) + if !ok { + return fmt.Errorf("you're not signed in — run 'createos login' to get started") + } + + // Everything after the directory should have been a flag, and the + // parser stopped reading them there. Say so before the empty-job-list + // error hides the real cause. + if c.Args().Len() > 1 { + if err := checkMisplacedFlags(c, c.Args().Slice()[1:]); err != nil { + return err + } + } + jobs, err := matrixJobs(c) + if err != nil { + return err + } + concurrency := c.Int("concurrency") + if concurrency < 1 { + return fmt.Errorf("--concurrency must be at least 1 (got %d)", concurrency) + } + opts, err := parseComposeOptions(c) + if err != nil { + return err + } + + ctx := c.Context + if opts.Timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, opts.Timeout) + defer cancel() + } + + quiet := output.IsJSON(c) + say := func(format string, a ...any) { + if !quiet { + pterm.Info.Printfln(format, a...) + } + } + + logDir, err := matrixLogDir(c.String("logs")) + if err != nil { + return err + } + + golden, cleanupGolden, err := matrixGoldenBox(ctx, c, client, opts, say) + if err != nil { + return err + } + defer cleanupGolden() + + // Pause the golden box once, here, rather than letting every job race + // to do it. Fork needs a paused source, and N concurrent pause calls + // on the same sandbox is a fight nobody needs to have. + // + // pauseForFork, not ensureForkable: matrix may pause this sandbox + // because it built it. With --from the sandbox belongs to the user, so + // say what is about to happen to it rather than doing it silently. + if ref := strings.TrimSpace(c.String("from")); ref != "" { + say("Pausing %s to take the snapshot the clones come from", golden) + } + if err := pauseForFork(ctx, client, golden); err != nil { + return err + } + say("Cloning %s into %d sandbox(es), %d at a time", golden, len(jobs), concurrency) + results := matrixRunJobs(ctx, client, golden, jobs, concurrency, logDir, opts, quiet) + + failed := 0 + for _, r := range results { + if r.ExitCode != 0 || r.Error != "" { + failed++ + } + } + + if quiet { + output.Render(c, map[string]any{ + "golden": golden, + "jobs": results, + "failed": failed, + }, func() {}) + } else { + matrixPrintSummary(results, logDir) + } + + if failed > 0 { + return cli.Exit("", 1) + } + return nil +} + +// matrixJobs collects the job commands from --job and --jobs-file. +func matrixJobs(c *cli.Context) ([]string, error) { + jobs := append([]string{}, c.StringSlice("job")...) + if path := c.String("jobs-file"); path != "" { + f, err := os.Open(path) // #nosec G304 -- the user named this file on their own command line + if err != nil { + return nil, fmt.Errorf("read --jobs-file: %w", err) + } + defer func() { _ = f.Close() }() //nolint:errcheck // read-only handle + scanner := bufio.NewScanner(f) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + jobs = append(jobs, line) + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("read --jobs-file: %w", err) + } + } + if len(jobs) == 0 { + return nil, errors.New( + "no jobs to run\n\n Give at least one --job, or a --jobs-file:\n createos sandbox matrix . --job 'bun test unit' --job 'bun test e2e'") + } + return jobs, nil +} + +func matrixLogDir(want string) (string, error) { + if want == "" { + return os.MkdirTemp("", "createos-matrix-*") + } + if err := os.MkdirAll(want, 0o750); err != nil { + return "", fmt.Errorf("create --logs directory: %w", err) + } + return filepath.Abs(want) +} + +// matrixGoldenBox returns the sandbox id to clone from, plus the cleanup +// that runs when the matrix finishes. +// +// Two ways in. --from names a sandbox the user already prepared, and +// matrix never destroys it — it did not create it. Otherwise matrix builds +// one from , runs --prepare, and owns its teardown. +func matrixGoldenBox( + ctx context.Context, + c *cli.Context, + client *api.SandboxClient, + opts *composeOptions, + say func(string, ...any), +) (string, func(), error) { + noop := func() {} + + if ref := strings.TrimSpace(c.String("from")); ref != "" { + if c.Args().Len() > 0 { + return "", noop, errors.New("--from and do the same job — give one or the other") + } + id, err := resolveSandboxRef(ctx, client, ref) + if err != nil { + return "", noop, err + } + if err := matrixRefuseDisks(ctx, client, id); err != nil { + return "", noop, err + } + return id, noop, nil + } + + dir := strings.TrimSpace(c.Args().First()) + if dir == "" { + return "", noop, errors.New( + "please give a directory to clone, or --from an existing sandbox\n\n Example:\n createos sandbox matrix . --prepare 'bun install' --job 'bun test'") + } + + tree, err := stageDir(ctx, dir, stageOptions{Exclude: opts.Exclude}) + if err != nil { + return "", noop, err + } + defer func() { _ = os.Remove(tree.Path) }() //nolint:errcheck // temp file + say("Packed %d file(s), %s", tree.Files, humanBytes(tree.Size)) + + if len(opts.Egress) == 0 { + say("Egress unrestricted — every clone can reach any host. Restrict it with --egress or --egress-preset.") + } + + sb, err := createComposeBox(ctx, client, opts) + if err != nil { + return "", noop, err + } + cleanup := func() { + if c.Bool("keep-golden") { + pterm.Info.Printfln("Golden sandbox %s kept. Destroy it with: createos sandbox rm --force %s", sb.ID, sb.ID) + return + } + destroyQuiet(ctx, client, sb.ID, func(msg string) { pterm.Warning.Println(msg) }) + } + + if err := shipTree(ctx, client, sb.ID, tree, composeWorkDir); err != nil { + cleanup() + return "", noop, err + } + say("Golden sandbox %s is up", sb.ID) + + if prepare := strings.TrimSpace(c.String("prepare")); prepare != "" { + say("Preparing once: %s", prepare) + var buf bytes.Buffer + res, err := runManaged(ctx, client, sb.ID, prepare, composeWorkDir, opts.Env, &buf) + if err != nil { + cleanup() + return "", noop, fmt.Errorf("prepare: %w", err) + } + if res.ExitCode != 0 { + cleanup() + return "", noop, fmt.Errorf("prepare exited %d — no clones were made\n\n%s", + res.ExitCode, strings.TrimSpace(buf.String())) + } + } + return sb.ID, cleanup, nil +} + +// matrixRefuseDisks stops a run that would silently lose data. A fork does +// not carry its source's S3 disk attachments (issue #63), so jobs would +// read an empty mount path and "pass" against nothing. +func matrixRefuseDisks(ctx context.Context, client *api.SandboxClient, id string) error { + disks, err := client.ListSandboxDisks(ctx, id) + if err != nil { + // Not being able to check is not a reason to refuse the run. + return nil //nolint:nilerr + } + if len(disks) == 0 { + return nil + } + return fmt.Errorf( + "sandbox %s has %d S3 disk(s) mounted, and a fork does not carry them (issue #63)\n\n The clones would run against empty mount paths.\n Copy what the jobs need into the sandbox's own filesystem first, then re-run", + id, len(disks)) +} + +// matrixRunJobs clones the golden sandbox once per job and runs them, at +// most `concurrency` at a time. +func matrixRunJobs( + ctx context.Context, + client *api.SandboxClient, + golden string, + jobs []string, + concurrency int, + logDir string, + opts *composeOptions, + quiet bool, +) []matrixJobResult { + results := make([]matrixJobResult, len(jobs)) + slots := make(chan struct{}, concurrency) + var wg sync.WaitGroup + + for i, job := range jobs { + wg.Add(1) + go func(i int, job string) { + defer wg.Done() + slots <- struct{}{} + defer func() { <-slots }() + results[i] = matrixRunOne(ctx, client, golden, i, job, logDir, opts) + if !quiet { + matrixPrintOne(results[i]) + } + }(i, job) + } + wg.Wait() + return results +} + +// matrixRunOne clones, runs one job, and destroys the clone. +func matrixRunOne( + ctx context.Context, + client *api.SandboxClient, + golden string, + index int, + job, logDir string, + opts *composeOptions, +) (res matrixJobResult) { + // Named return, deliberately. The teardown below runs in a defer, and + // with an unnamed return Go copies the result value before defers run, + // so a failed destroy would never reach the caller and the matrix + // would exit 0 having leaked a clone. + res = matrixJobResult{Index: index, Cmd: job, ExitCode: -1} + logPath := filepath.Join(logDir, fmt.Sprintf("job-%d.log", index)) + res.Log = logPath + + logFile, err := os.Create(logPath) // #nosec G304 -- logDir is ours or the user's own --logs + if err != nil { + res.Error = err.Error() + return res + } + defer func() { _ = logFile.Close() }() //nolint:errcheck // the job result carries the real error + + // Every clone comes from the same paused snapshot, so they are made + // one at a time and resumed on the spot. + forks, err := forkN(ctx, client, golden, api.SandboxForkReq{Egress: opts.Egress}, 1, nil) + if err != nil { + res.Error = err.Error() + // A fork that was created but never settled is running and + // billable, and this job is the only thing that knows its id. + var leak *forkLeak + if errors.As(err, &leak) { + for _, id := range leak.IDs { + destroyQuiet(ctx, client, id, func(msg string) { + res.Error += "\n " + msg + }) + } + } + return res + } + clone := forks[0] + res.Sandbox = clone.ID + defer destroyQuiet(ctx, client, clone.ID, func(msg string) { + // A teardown failure must reach the result. The job may have + // passed, but a clone nobody destroyed keeps billing, and + // swallowing this is how that goes unnoticed. + if res.Error == "" { + res.Error = msg + } + }) + + // A fork does not inherit its source's auto-pause — measured: a golden + // box with auto_pause=900 produced clones with auto_pause=None (and a + // different shape). Without this, a matrix that dies mid-run leaves + // every clone running with nothing left to stop it. + if opts.AutoPause > 0 { + secs := int(opts.AutoPause.Seconds()) + if _, pauseErr := client.SetAutoPause(ctx, clone.ID, &secs); pauseErr != nil { + res.Error = fmt.Sprintf("could not set the auto-pause backstop on %s: %v", clone.ID, pauseErr) + return res + } + } + + out, err := runManaged(ctx, client, clone.ID, job, composeWorkDir, opts.Env, logFile) + if err != nil { + res.Error = err.Error() + return res + } + res.ExitCode = out.ExitCode + res.DurationMs = out.Duration.Milliseconds() + return res +} + +func matrixPrintOne(r matrixJobResult) { + switch { + case r.Error != "": + pterm.Error.Printfln("job %d failed to run: %s", r.Index, r.Error) + case r.ExitCode == 0: + pterm.Success.Printfln("job %d ok in %s — %s", r.Index, + time.Duration(r.DurationMs)*time.Millisecond, r.Cmd) + default: + pterm.Error.Printfln("job %d exited %d — %s", r.Index, r.ExitCode, r.Cmd) + } +} + +func matrixPrintSummary(results []matrixJobResult, logDir string) { + rows := make([][]string, 0, 1+len(results)) + rows = append(rows, []string{"JOB", "EXIT", "TIME", "COMMAND"}) + for _, r := range results { + exit := fmt.Sprintf("%d", r.ExitCode) + if r.Error != "" { + exit = "error" + } + rows = append(rows, []string{ + fmt.Sprintf("%d", r.Index), + exit, + (time.Duration(r.DurationMs) * time.Millisecond).Round(time.Millisecond).String(), + r.Cmd, + }) + } + _ = pterm.DefaultTable.WithHasHeader().WithData(rows).Render() //nolint:errcheck // a failed table render must not change the exit code + fmt.Printf("\nLogs: %s\n", logDir) +} diff --git a/cmd/sandbox/offload.go b/cmd/sandbox/offload.go new file mode 100644 index 0000000..403098f --- /dev/null +++ b/cmd/sandbox/offload.go @@ -0,0 +1,343 @@ +package sandbox + +import ( + "archive/tar" + "context" + "errors" + "fmt" + "io" + "os" + "path" + "path/filepath" + "slices" + "strings" + "time" + + "github.com/pterm/pterm" + "github.com/urfave/cli/v2" + + "github.com/NodeOps-app/createos-cli/internal/api" + "github.com/NodeOps-app/createos-cli/internal/output" +) + +func newOffloadCommand() *cli.Command { + return &cli.Command{ + Name: "offload", + Usage: "Run one command on a throwaway sandbox, then destroy it", + ArgsUsage: " -- ", + Description: `Offload moves one piece of work off your machine. It creates a sandbox, +uploads the directory, runs the command inside it, brings back anything you +asked for, and destroys the sandbox — whether the command passed or failed. + +Use it for work with a finish line: a test suite, a build, a migration, a +script you did not write. For work that must outlive one command — a dev +server, a watcher — create a sandbox and keep it instead. + +The upload honours .gitignore, so node_modules and build output stay on your +machine. Outside a git repository a fixed skip-list stands in. + +The command's exit code becomes this command's exit code. + +Flags must come before the directory. Anything after it belongs to the +command you are running. + +Examples: + # Run a test suite somewhere else + createos sandbox offload . -- bun test + + # Lock the sandbox to the npm registry and nothing else + createos sandbox offload --egress-preset npm . -- npm ci + + # Bring the coverage report back + createos sandbox offload --fetch coverage . -- bun test --coverage + + # Keep the sandbox when the command fails, so you can shell in and look + createos sandbox offload --keep-on-fail . -- make build`, + Flags: append(composeFlags(), + &cli.StringSliceFlag{ + Name: "fetch", + Usage: "Path inside the work directory to download when the command finishes (repeatable)", + }, + &cli.BoolFlag{ + Name: "keep-on-fail", + Usage: "Leave the sandbox alive when the command exits non-zero, so you can inspect it", + }, + ), + Action: runOffload, + } +} + +func runOffload(c *cli.Context) error { + client, ok := c.App.Metadata[api.SandboxClientKey].(*api.SandboxClient) + if !ok { + return fmt.Errorf("you're not signed in — run 'createos login' to get started") + } + + dir, cmd, err := splitDirAndCommand(c) + if err != nil { + return err + } + opts, err := parseComposeOptions(c) + if err != nil { + return err + } + + ctx := c.Context + if opts.Timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, opts.Timeout) + defer cancel() + } + + quiet := output.IsJSON(c) + say := func(format string, a ...any) { + if !quiet { + pterm.Info.Printfln(format, a...) + } + } + + tree, err := stageDir(ctx, dir, stageOptions{Exclude: opts.Exclude}) + if err != nil { + return err + } + defer func() { _ = os.Remove(tree.Path) }() //nolint:errcheck // temp file; removal failure is benign + say("Packed %d file(s), %s", tree.Files, humanBytes(tree.Size)) + + if len(opts.Egress) == 0 { + say("Egress unrestricted — this sandbox can reach any host. Restrict it with --egress or --egress-preset.") + } + + sb, err := createComposeBox(ctx, client, opts) + if err != nil { + return err + } + say("Sandbox %s is up", sb.ID) + + destroyed := false + defer func() { + if !destroyed { + destroyQuiet(ctx, client, sb.ID, func(msg string) { pterm.Warning.Println(msg) }) + } + }() + + if shipErr := shipTree(ctx, client, sb.ID, tree, composeWorkDir); shipErr != nil { + return shipErr + } + + out := io.Writer(os.Stdout) + if quiet { + out = io.Discard + } + res, err := runManaged(ctx, client, sb.ID, cmd, composeWorkDir, opts.Env, out) + if err != nil { + return err + } + + if len(c.StringSlice("fetch")) > 0 { + if err := fetchPaths(ctx, client, sb.ID, composeWorkDir, c.StringSlice("fetch"), dir); err != nil { + return fmt.Errorf("fetch results: %w", err) + } + say("Fetched %s into %s", strings.Join(c.StringSlice("fetch"), ", "), dir) + } + + teardownFailure := "" + if res.ExitCode != 0 && c.Bool("keep-on-fail") { + destroyed = true + pterm.Warning.Printfln("Command exited %d. Sandbox %s kept.", res.ExitCode, sb.ID) + fmt.Printf(" Look around: createos sandbox shell %s\n", sb.ID) + fmt.Printf(" Destroy it: createos sandbox rm --force %s\n", sb.ID) + } else { + destroyQuiet(ctx, client, sb.ID, func(msg string) { teardownFailure = msg }) + destroyed = true + } + + if quiet { + output.Render(c, map[string]any{ + "sandbox": sb.ID, + "command": cmd, + "exit_code": res.ExitCode, + "duration_ms": res.Duration.Milliseconds(), + "teardown_failure": teardownFailure, + }, func() {}) + } else if res.ExitCode == 0 && teardownFailure == "" { + pterm.Success.Printfln("Done in %s", res.Duration.Round(time.Millisecond)) + } + + // A teardown failure has to change the exit code. `offload` promises a + // throwaway sandbox; a leaked one keeps billing, and a CI job that only + // reads the exit status would call this a clean run and never find out. + if teardownFailure != "" { + return fmt.Errorf( + "the command exited %d, but the sandbox was not destroyed: %s\n\n It is still billable. Remove it with:\n createos sandbox rm --force %s", + res.ExitCode, teardownFailure, sb.ID) + } + if res.ExitCode != 0 { + return cli.Exit("", res.ExitCode) + } + return nil +} + +// splitDirAndCommand pulls and the command out of the argument +// list: the first argument is the directory, the rest is the command. +// +// urfave/cli hands `--` through as an ordinary argument rather than eating +// it, so a literal "--" would end up at the front of the command and the +// shell would fail on it. Dropping it here means both spellings work — +// `offload . -- bun test` and `offload . bun test` — which matters because +// the separator is the single most common thing to get wrong on a command +// shaped like this one. +func splitDirAndCommand(c *cli.Context) (dir, cmd string, err error) { + args := c.Args().Slice() + if len(args) < 2 { + return "", "", errors.New( + "please give a directory and a command\n\n Example:\n createos sandbox offload . -- bun test") + } + dir = args[0] + rest := args[1:] + // Anything between the directory and `--` was meant as a flag and was + // not read as one. Catch it before it is pasted into the command line + // and the shell reports something unrelated. + if sep := slices.Index(rest, "--"); sep > 0 { + if flagErr := checkMisplacedFlags(c, rest[:sep]); flagErr != nil { + return "", "", flagErr + } + } + if rest[0] == "--" { + rest = rest[1:] + } + cmd = strings.TrimSpace(strings.Join(rest, " ")) + if cmd == "" { + return "", "", errors.New( + "the command is empty\n\n Example:\n createos sandbox offload . -- bun test") + } + return dir, cmd, nil +} + +// fetchPaths downloads the named paths out of the sandbox and unpacks them +// under localRoot, keeping their relative layout. +// +// One tar for the whole set, not one download per path: the file API moves +// a single stream far better than N round trips, and it keeps directory +// trees intact. +// +// Never point this at a mounted S3 disk. Reading a disk mount through the +// file API is a known crash (issue #71) — copy what you need out of the +// mount from inside the sandbox first. +func fetchPaths(ctx context.Context, client *api.SandboxClient, sandboxID, remoteRoot string, paths []string, localRoot string) error { + quoted := make([]string, 0, len(paths)) + for _, p := range paths { + p = strings.TrimPrefix(strings.TrimSpace(p), "/") + if p == "" || strings.Contains(p, "..") { + return fmt.Errorf("--fetch %q must be a path inside the work directory", p) + } + quoted = append(quoted, shellQuote(p)) + } + const remoteTar = "/tmp/createos-fetch.tar" + pack := fmt.Sprintf("set -eu\ncd %s\ntar -cf %s %s\n", + shellQuote(remoteRoot), remoteTar, strings.Join(quoted, " ")) + resp, err := client.ExecSandbox(ctx, sandboxID, api.SandboxExecReq{Cmd: "bash", Args: []string{"-lc", pack}}) + if err != nil { + return err + } + if resp.Result.ExitCode != 0 { + return fmt.Errorf("packing the requested paths exited %d: %s", + resp.Result.ExitCode, strings.TrimSpace(resp.Result.Stderr)) + } + + tmp, err := os.CreateTemp("", "createos-fetch-*.tar") + if err != nil { + return err + } + defer func() { + _ = tmp.Close() //nolint:errcheck // read path below owns the error + _ = os.Remove(tmp.Name()) //nolint:errcheck // temp file + }() + if _, err := client.DownloadFile(ctx, sandboxID, remoteTar, tmp); err != nil { + return err + } + if _, err := tmp.Seek(0, io.SeekStart); err != nil { + return err + } + return untarInto(tmp, localRoot) +} + +// untarInto extracts r under root, refusing any entry that would escape it. +// +// Every write goes through os.Root, which resolves names relative to an +// open directory descriptor and refuses any component that leaves the +// root — a symlink included. A lexical prefix check is not enough here: +// it validates the pathname this code builds, while MkdirAll and OpenFile +// still follow a symlink that already exists on the caller's disk. A repo +// holding `coverage -> /etc` plus a sandbox-built entry `coverage/passwd` +// is enough to write outside the tree (CWE-22, CWE-59). The archive is +// produced inside a sandbox that ran code the user did not write, so it +// is untrusted by construction. +func untarInto(r io.Reader, root string) error { + if err := os.MkdirAll(root, 0o750); err != nil { + return err + } + rootDir, err := os.OpenRoot(root) + if err != nil { + return err + } + defer func() { _ = rootDir.Close() }() //nolint:errcheck // read side owns the real error + + tr := tar.NewReader(r) + for { + hdr, nextErr := tr.Next() + if errors.Is(nextErr, io.EOF) { + return nil + } + if nextErr != nil { + return nextErr + } + name := path.Clean("/" + filepath.ToSlash(hdr.Name)) + name = strings.TrimPrefix(name, "/") + if name == "" || name == "." { + continue + } + switch hdr.Typeflag { + case tar.TypeDir: + if err := rootDir.MkdirAll(name, 0o750); err != nil { + return fmt.Errorf("archive entry %q: %w", hdr.Name, err) + } + case tar.TypeReg: + if dir := path.Dir(name); dir != "." { + if err := rootDir.MkdirAll(dir, 0o750); err != nil { + return fmt.Errorf("archive entry %q: %w", hdr.Name, err) + } + } + if err := writeFetchedFile(rootDir, tr, name, hdr.FileInfo().Mode()); err != nil { + return fmt.Errorf("archive entry %q: %w", hdr.Name, err) + } + default: + // Symlinks and devices out of a sandbox have no safe meaning + // on the caller's disk. Skip them rather than guess. + continue + } + } +} + +// fetchFileMaxBytes bounds one extracted file. A sandbox-built archive is +// untrusted, and an unbounded copy is a decompression bomb (CWE-409). +const fetchFileMaxBytes int64 = 2 << 30 + +func writeFetchedFile(rootDir *os.Root, r io.Reader, name string, mode os.FileMode) error { + // rootDir resolves name against an open directory descriptor, so a + // symlink anywhere in the path cannot reach outside the extraction + // root. One that stays inside it is harmless: the write still lands in + // the tree the caller asked for. + f, err := rootDir.OpenFile(name, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, mode.Perm()&0o755) + if err != nil { + return err + } + defer func() { _ = f.Close() }() //nolint:errcheck // the copy error below is the one that matters + written, err := io.Copy(f, io.LimitReader(r, fetchFileMaxBytes)) + if err != nil { + return err + } + if written == fetchFileMaxBytes { + return fmt.Errorf("%s is over the %s per-file fetch limit", name, humanBytes(fetchFileMaxBytes)) + } + return nil +} diff --git a/cmd/sandbox/pull.go b/cmd/sandbox/pull.go index 28ca621..806e24d 100644 --- a/cmd/sandbox/pull.go +++ b/cmd/sandbox/pull.go @@ -55,6 +55,14 @@ func runPull(c *cli.Context) error { return err } + mount, err := diskMountBlocksFileAPI(c.Context, client, id, remote) + if err != nil { + return err + } + if mount != "" { + return diskMountFileAPIError(remote, mount, "pull") + } + f, err := os.Create(local) // #nosec G304 -- local is a user-supplied destination path if err != nil { return fmt.Errorf("could not create %s: %w", local, err) diff --git a/cmd/sandbox/push.go b/cmd/sandbox/push.go index a8a6d1b..7ca7065 100644 --- a/cmd/sandbox/push.go +++ b/cmd/sandbox/push.go @@ -58,6 +58,13 @@ func runPush(c *cli.Context) error { if err != nil { return err } + mount, err := diskMountBlocksFileAPI(c.Context, client, id, remote) + if err != nil { + return err + } + if mount != "" { + return diskMountFileAPIError(remote, mount, "push") + } // Open the source: a real file (we know its size for Content-Length) // or stdin ("-") for piped uploads. diff --git a/cmd/sandbox/sandbox.go b/cmd/sandbox/sandbox.go index 5353359..cd67973 100644 --- a/cmd/sandbox/sandbox.go +++ b/cmd/sandbox/sandbox.go @@ -15,6 +15,8 @@ func NewSandboxCommand() *cli.Command { Usage: "Manage sandboxes", Subcommands: []*cli.Command{ newRunCommand(), + newOffloadCommand(), + newMatrixCommand(), newCreateCommand(), newListCommand(), newGetCommand(), @@ -23,6 +25,7 @@ func NewSandboxCommand() *cli.Command { newPauseCommand(), newResumeCommand(), newForkCommand(), + newSelfCommand(), newExecCommand(), newProcessCommand(), newPushCommand(), diff --git a/cmd/sandbox/self.go b/cmd/sandbox/self.go new file mode 100644 index 0000000..58ae27d --- /dev/null +++ b/cmd/sandbox/self.go @@ -0,0 +1,206 @@ +package sandbox + +import ( + "context" + "errors" + "fmt" + "net" + "net/http" + "net/url" + "os" + "strings" + "time" + + "github.com/pterm/pterm" + "github.com/urfave/cli/v2" + + "github.com/NodeOps-app/createos-cli/internal/output" + "github.com/NodeOps-app/createos-cli/internal/terminal" +) + +// The guest agent listens on loopback inside every sandbox. Loopback-only +// is the whole security model: nothing outside the sandbox can reach it, +// so it needs no credential — and a sandbox can only ever signal itself. +const ( + selfSignalAddr = "127.0.0.1:1029" + selfFifoPath = "/run/self" + selfDialWait = 2 * time.Second +) + +// selfSignalAddrForTest is the address selfSignalHTTP dials. It is a +// variable purely so a test can point it at a stub agent; nothing else +// ever reassigns it. +var selfSignalAddrForTest = selfSignalAddr + +func newSelfCommand() *cli.Command { + return &cli.Command{ + Name: "self", + Usage: "Pause or delete the sandbox this command is running inside", + Description: `Self-signal lets a workload end its own sandbox from the inside. + +Run these INSIDE a sandbox, not on your laptop. There is no sandbox id to +pass and no API key involved: the agent listens on loopback only, so the +only sandbox you can signal is the one you are in. + +Use it when a job knows it is finished long before anything outside does — +a batch run, a CI job, a one-shot agent task. The machine is released the +moment the last line executes, with no polling loop and no credential +inside the sandbox. + +Examples: + # Park this sandbox; resume it later from outside + createos sandbox self pause --reason job-complete + + # Destroy this sandbox. Irreversible. + createos sandbox self delete + +Without this CLI, the same signals are one line each: + curl -X POST http://127.0.0.1:1029/self/pause + echo park > /run/self`, + Subcommands: []*cli.Command{ + { + Name: "pause", + Usage: "Pause this sandbox, keeping its disk and memory", + Flags: selfFlags(), + Action: runSelf("pause"), + }, + { + Name: "delete", + Aliases: []string{"destroy", "rm"}, + Usage: "Destroy this sandbox. Irreversible", + Flags: append(selfFlags(), &cli.BoolFlag{ + Name: "force", + Aliases: []string{"f", "yes", "y"}, + Usage: "Skip the confirmation prompt", + }), + Action: runSelf("delete"), + }, + }, + } +} + +func selfFlags() []cli.Flag { + return []cli.Flag{ + &cli.StringFlag{ + Name: "reason", + Usage: "Free-text label recorded with the signal (truncated to 128 characters)", + }, + } +} + +func runSelf(action string) cli.ActionFunc { + return func(c *cli.Context) error { + // Destroying a sandbox cannot be undone, and this command is most + // often typed inside a shell on a box someone is still using. + if action == "delete" && !c.Bool("force") { + if !terminal.IsInteractive() { + return errors.New( + "deleting this sandbox is irreversible — pass --force to confirm\n\n Example:\n createos sandbox self delete --force") + } + ok, err := pterm.DefaultInteractiveConfirm. + WithDefaultText("Destroy this sandbox? Everything on it is lost"). + Show() + if err != nil { + return err + } + if !ok { + fmt.Println("Cancelled. Nothing changed.") + return nil + } + } + + reason := strings.TrimSpace(c.String("reason")) + if err := sendSelfSignal(c.Context, action, reason); err != nil { + return err + } + + if output.IsJSON(c) { + output.Render(c, map[string]any{"status": "accepted", "action": action, "reason": reason}, func() {}) + return nil + } + switch action { + case "pause": + pterm.Success.Println("Pause accepted. This sandbox is being snapshotted.") + fmt.Println(" Bring it back from outside with: createos sandbox resume ") + default: + pterm.Success.Println("Delete accepted. This sandbox is going away.") + } + return nil + } +} + +// sendSelfSignal delivers one signal to the guest agent, preferring HTTP +// and falling back to the FIFO. +// +// Both surfaces exist because the FIFO works in images with no curl and +// no working loopback HTTP stack. Trying HTTP first keeps the useful part +// of the failure — the agent answers with a status code — and only drops +// to the pipe, which is fire-and-forget, when HTTP is not there at all. +func sendSelfSignal(ctx context.Context, action, reason string) error { + httpErr := selfSignalHTTP(ctx, action, reason) + if httpErr == nil { + return nil + } + if fifoErr := selfSignalFIFO(action); fifoErr == nil { + return nil + } + return notInsideSandboxError(action, httpErr) +} + +func selfSignalHTTP(ctx context.Context, action, reason string) error { + endpoint := "http://" + selfSignalAddrForTest + "/self/" + action + if reason != "" { + endpoint += "?reason=" + url.QueryEscape(reason) + } + req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, nil) + if err != nil { + return err + } + client := &http.Client{Timeout: selfDialWait} + resp, err := client.Do(req) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() //nolint:errcheck // status code is what matters here + // The agent answers 202 Accepted and then acts. Anything else means + // something is listening on that port that is not the guest agent. + if resp.StatusCode != http.StatusAccepted && resp.StatusCode != http.StatusOK { + return fmt.Errorf("the agent on %s answered %s", selfSignalAddrForTest, resp.Status) + } + return nil +} + +// selfSignalFIFO writes one verb to /run/self. The pipe takes no reason, +// so a reason given on the command line is dropped on this path. +func selfSignalFIFO(action string) error { + verb := "park" + if action == "delete" { + verb = "retire" + } + f, err := os.OpenFile(selfFifoPath, os.O_WRONLY, 0) + if err != nil { + return err + } + defer func() { _ = f.Close() }() //nolint:errcheck // the write error below is the one that matters + _, err = f.WriteString(verb + "\n") + return err +} + +// notInsideSandboxError is the message for the most likely mistake: +// running this on a laptop. Naming the alternative matters, because the +// command that does work from outside takes a sandbox id and this one +// does not. +func notInsideSandboxError(action string, cause error) error { + var opErr *net.OpError + inside := "no agent is listening" + if errors.As(cause, &opErr) || strings.Contains(cause.Error(), "connection refused") { + inside = "nothing answered on " + selfSignalAddr + } + outside := "pause" + if action == "delete" { + outside = "rm --force" + } + return fmt.Errorf( + "could not signal this sandbox — %s\n\n 'sandbox self' only works INSIDE a sandbox.\n From your own machine, name the sandbox instead:\n createos sandbox %s \n\n Underlying error: %w", + inside, outside, cause) +} diff --git a/cmd/sandbox/stage.go b/cmd/sandbox/stage.go new file mode 100644 index 0000000..98a1e3d --- /dev/null +++ b/cmd/sandbox/stage.go @@ -0,0 +1,287 @@ +package sandbox + +import ( + "archive/tar" + "context" + "fmt" + "io" + "io/fs" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/NodeOps-app/createos-cli/internal/api" +) + +// stageDefaultExcludes are directories a build regenerates. They are big, +// they are usually the largest thing in a tree, and shipping them is the +// difference between a 2 MB upload and a 900 MB one. This list only +// applies outside a git repository — inside one, .gitignore already says +// what belongs, and it says it better than any fixed list can. +var stageDefaultExcludes = []string{ + ".git", "node_modules", "target", "__pycache__", ".venv", "venv", + "dist", "build", ".next", ".turbo", ".cache", "vendor", +} + +// stageMaxBytes caps the upload. The file API refuses more than 500 MB, +// and hitting that limit after a two-minute upload is a bad way to find +// out. Failing early with the measured size names the problem instead. +const stageMaxBytes int64 = 500 << 20 + +// stageOptions tunes what stageDir packs. +type stageOptions struct { + // IncludeGit ships the .git directory. Orca needs it (its remote git + // reads the history); offload and matrix do not, and it is often the + // bulk of the payload. + IncludeGit bool + // Exclude adds path prefixes to skip, on top of .gitignore. + Exclude []string +} + +// stagedTree is a packed directory ready to upload. +type stagedTree struct { + Path string // temp tar on the local disk; the caller removes it + Size int64 + Files int +} + +// stageDir packs dir into a tar file on local disk and returns its path. +// UploadFile needs the length up front, so the archive is staged rather +// than streamed. +// +// Inside a git repository the file list comes from `git ls-files --cached +// --others --exclude-standard`: tracked files plus untracked ones that +// .gitignore does not exclude. That is "what the user sees on their +// laptop" minus the build output, and it needs no exclude list of ours. +// Outside a repository there is no such signal, so stageDefaultExcludes +// stands in. +func stageDir(ctx context.Context, dir string, opts stageOptions) (*stagedTree, error) { + abs, err := filepath.Abs(dir) + if err != nil { + return nil, fmt.Errorf("resolve %s: %w", dir, err) + } + info, err := os.Stat(abs) // #nosec G703 -- abs comes from filepath.Abs of a user-named directory + if err != nil { + return nil, fmt.Errorf("no such directory: %s", dir) + } + if !info.IsDir() { + return nil, fmt.Errorf("%s is a file, not a directory", dir) + } + + paths, err := stageFileList(ctx, abs, opts) + if err != nil { + return nil, err + } + if len(paths) == 0 { + return nil, fmt.Errorf("%s has nothing to send — every file in it is ignored or excluded", dir) + } + + tmp, err := os.CreateTemp("", "createos-stage-*.tar") + if err != nil { + return nil, err + } + defer func() { _ = tmp.Close() }() //nolint:errcheck // the Stat below owns the real error + + tw := tar.NewWriter(tmp) + for _, rel := range paths { + if err = stageTarAppend(tw, abs, rel); err != nil { + _ = tw.Close() //nolint:errcheck // already unwinding + _ = os.Remove(tmp.Name()) //nolint:errcheck // best-effort cleanup + return nil, err + } + } + if err = tw.Close(); err != nil { + _ = os.Remove(tmp.Name()) //nolint:errcheck // best-effort cleanup + return nil, err + } + st, err := tmp.Stat() + if err != nil { + _ = os.Remove(tmp.Name()) //nolint:errcheck // best-effort cleanup + return nil, err + } + if st.Size() > stageMaxBytes { + _ = os.Remove(tmp.Name()) //nolint:errcheck // best-effort cleanup + return nil, fmt.Errorf( + "%s packs to %s, over the %s upload limit\n\n Exclude what the sandbox does not need:\n --exclude (repeatable)", + dir, humanBytes(st.Size()), humanBytes(stageMaxBytes)) + } + return &stagedTree{Path: tmp.Name(), Size: st.Size(), Files: len(paths)}, nil +} + +// stageFileList returns the repo-relative paths to pack. +func stageFileList(ctx context.Context, abs string, opts stageOptions) ([]string, error) { + paths, gitErr := stageGitFileList(ctx, abs) + if gitErr != nil { + var walkErr error + if paths, walkErr = stageWalkFileList(abs); walkErr != nil { + return nil, walkErr + } + } else if opts.IncludeGit { + // git ls-files never lists .git itself, so walk it separately. + gitDir, err := stageWalkDir(abs, ".git") + if err != nil { + return nil, err + } + paths = append(paths, gitDir...) + } + if len(opts.Exclude) == 0 { + return paths, nil + } + kept := paths[:0] + for _, p := range paths { + if !stageExcluded(p, opts.Exclude) { + kept = append(kept, p) + } + } + return kept, nil +} + +// stageGitFileList asks git what the working tree holds. A non-nil error +// means "not a git repository" (or no git binary), not a hard failure. +func stageGitFileList(ctx context.Context, abs string) ([]string, error) { + out, err := exec.CommandContext(ctx, "git", "-C", abs, //#nosec G204,G702 -- abs is filepath.Abs of a user-named directory + "ls-files", "-z", "--cached", "--others", "--exclude-standard").Output() + if err != nil { + return nil, err + } + paths := make([]string, 0, 4096) + for _, p := range strings.Split(string(out), "\x00") { + if p != "" { + paths = append(paths, p) + } + } + return paths, nil +} + +// stageWalkFileList is the non-git fallback: walk the tree, skipping the +// directories a build regenerates. +func stageWalkFileList(abs string) ([]string, error) { + var paths []string + err := filepath.WalkDir(abs, func(p string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return nil //nolint:nilerr // an unreadable entry is not worth failing the whole stage + } + rel, relErr := filepath.Rel(abs, p) + if relErr != nil || rel == "." { + return nil //nolint:nilerr + } + rel = filepath.ToSlash(rel) + if d.IsDir() { + if stageExcluded(rel, stageDefaultExcludes) { + return filepath.SkipDir + } + return nil + } + paths = append(paths, rel) + return nil + }) + return paths, err +} + +// stageWalkDir collects every file under abs/sub, relative to abs. +func stageWalkDir(abs, sub string) ([]string, error) { + var paths []string + err := filepath.WalkDir(filepath.Join(abs, sub), func(p string, d fs.DirEntry, walkErr error) error { + if walkErr != nil || d.IsDir() { + return nil //nolint:nilerr + } + if rel, relErr := filepath.Rel(abs, p); relErr == nil { + paths = append(paths, filepath.ToSlash(rel)) + } + return nil + }) + return paths, err +} + +// stageExcluded reports whether rel is excluded. +// +// An exclude matches two ways: as a path prefix ("build/out" excludes +// "build/out/app.js"), or as any single segment of the path +// ("node_modules" excludes "src/node_modules/x.js"). The segment rule is +// the one that matters — a monorepo has a node_modules under every +// package, and a user who writes --exclude node_modules means all of them. +func stageExcluded(rel string, excludes []string) bool { + segments := strings.Split(rel, "/") + for _, ex := range excludes { + ex = strings.Trim(filepath.ToSlash(ex), "/") + if ex == "" { + continue + } + if rel == ex || strings.HasPrefix(rel, ex+"/") { + return true + } + if !strings.Contains(ex, "/") { + for _, seg := range segments { + if seg == ex { + return true + } + } + } + } + return false +} + +func stageTarAppend(tw *tar.Writer, root, rel string) error { + abs := filepath.Join(root, rel) + info, err := os.Lstat(abs) // #nosec G703 -- rel comes from git ls-files or a walk of root, never ".." + if err != nil { + return nil //nolint:nilerr // a file deleted mid-walk is not fatal + } + link := "" + if info.Mode()&os.ModeSymlink != 0 { + if link, err = os.Readlink(abs); err != nil { + return nil //nolint:nilerr + } + } else if !info.Mode().IsRegular() { + return nil // sockets, fifos, devices have no place in a checkout + } + hdr, err := tar.FileInfoHeader(info, link) + if err != nil { + return err + } + hdr.Name = filepath.ToSlash(rel) + if err = tw.WriteHeader(hdr); err != nil { + return err + } + if link != "" || !info.Mode().IsRegular() { + return nil + } + f, err := os.Open(abs) // #nosec G304,G703 -- see the Lstat note above + if err != nil { + return nil //nolint:nilerr + } + defer func() { _ = f.Close() }() //nolint:errcheck // read-only handle + _, err = io.Copy(tw, f) + return err +} + +// shipTree uploads a staged tar and unpacks it at remoteDir inside the +// sandbox. The tar is removed from the sandbox afterwards so it does not +// double the payload's footprint on the box's disk — and, for matrix, so +// it is not copied into every fork. +func shipTree(ctx context.Context, client *api.SandboxClient, id string, tree *stagedTree, remoteDir string) error { + f, err := os.Open(tree.Path) // #nosec G304,G703 -- path is from os.CreateTemp + if err != nil { + return fmt.Errorf("open staged tar: %w", err) + } + defer func() { _ = f.Close() }() //nolint:errcheck // read-only handle + + const remoteTar = "/tmp/createos-stage.tar" + start := time.Now() + if upErr := client.UploadFile(ctx, id, remoteTar, f, tree.Size); upErr != nil { + return fmt.Errorf("upload %s to %s after %s: %w", + humanBytes(tree.Size), id, time.Since(start).Round(time.Millisecond), upErr) + } + unpack := fmt.Sprintf("set -eu\nmkdir -p %s\ntar -xf %s -C %s\nrm -f %s\n", + shellQuote(remoteDir), remoteTar, shellQuote(remoteDir), remoteTar) + resp, err := client.ExecSandbox(ctx, id, api.SandboxExecReq{Cmd: "bash", Args: []string{"-lc", unpack}}) + if err != nil { + return fmt.Errorf("unpack in %s: %w", id, err) + } + if resp.Result.ExitCode != 0 { + return fmt.Errorf("unpack in %s exited %d: %s", id, resp.Result.ExitCode, strings.TrimSpace(resp.Result.Stderr)) + } + return nil +} diff --git a/internal/api/client.go b/internal/api/client.go index 30516d2..c2c9d0e 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -2,11 +2,14 @@ package api import ( + "errors" "fmt" "io" "log" + "net" "net/http" "strings" + "time" "github.com/go-resty/resty/v2" ) @@ -31,7 +34,9 @@ func installAuthRefresh(client *resty.Client, authHeader string, refresher Token if refresher == nil { return } - client.SetRetryCount(1) + // The retry budget itself belongs to installTransientRetry, which every + // constructor installs. This condition only adds one more reason to + // spend an attempt, and Attempt==1 below keeps it to a single refresh. client.AddRetryCondition(func(resp *resty.Response, _ error) bool { if resp == nil || resp.StatusCode() != http.StatusUnauthorized { return false @@ -55,13 +60,72 @@ func installAuthRefresh(client *resty.Client, authHeader string, refresher Token }) } +// Retry budget shared by installTransientRetry and installAuthRefresh. +// Three attempts covers a single flaky hop without turning a real outage +// into a long stall. +const ( + transientRetryCount = 3 + transientRetryWait = 300 * time.Millisecond + transientRetryMaxWait = 3 * time.Second +) + +// installTransientRetry retries the failures a retry can actually fix. +// One dropped connection used to kill a whole command; a fan-out over N +// sandboxes multiplies that exposure by N, so this is the difference +// between a flaky run and a failed one. +// +// What retries, and why the split by method: +// +// - A connection that was never established (DNS failure, dial timeout, +// connection refused). The request provably never reached the server, +// so replaying it cannot duplicate anything. Safe for every method, +// POST included. +// - A 429 or 5xx answer, but only for methods with no side effect. A +// POST that got a 500 may well have created the sandbox before it +// failed, and a retry would leak a second one that nobody destroys. +// Leaking billable machines is worse than surfacing the error. +func installTransientRetry(client *resty.Client) { + client.SetRetryCount(transientRetryCount) + client.SetRetryWaitTime(transientRetryWait) + client.SetRetryMaxWaitTime(transientRetryMaxWait) + client.AddRetryCondition(func(resp *resty.Response, err error) bool { + if err != nil { + return isConnectSetupError(err) + } + if resp == nil { + return false + } + switch resp.Request.Method { + case http.MethodGet, http.MethodHead, http.MethodOptions: + default: + return false + } + return resp.StatusCode() == http.StatusTooManyRequests || resp.StatusCode() >= http.StatusInternalServerError + }) +} + +// isConnectSetupError reports whether err failed before any bytes reached +// the server. Only "dial" operations qualify: a read or write error means +// the request was already on the wire and may have been acted on. +func isConnectSetupError(err error) bool { + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) { + return true + } + var opErr *net.OpError + if errors.As(err, &opErr) { + return opErr.Op == "dial" + } + return false +} + // Auth header names. HTTP header keys are case-insensitive (and Go // canonicalises them on the wire), so these double as the API-key and // OAuth-access-token headers for both the main API and the fc-spawn // sandbox API. const ( - headerAPIKey = "X-Api-Key" // #nosec G101 -- HTTP header name, not a credential - headerAccessToken = "X-Access-Token" // #nosec G101 -- HTTP header name, not a credential + headerAPIKey = "X-Api-Key" // #nosec G101 -- HTTP header name, not a credential // pragma: allowlist secret + headerAccessToken = "X-Access-Token" // #nosec G101 -- HTTP header name, not a credential // pragma: allowlist secret ) // DefaultBaseURL is the default CreateOS API base URL. @@ -91,6 +155,8 @@ func NewClient(token, apiURL string, debug bool) APIClient { }) } + installTransientRetry(client) + return APIClient{Client: client} } @@ -115,6 +181,7 @@ func NewClientWithAccessToken(accessToken, apiURL string, debug bool, refresher }) } + installTransientRetry(client) installAuthRefresh(client, headerAccessToken, refresher) return APIClient{Client: client} diff --git a/internal/api/client_retry_test.go b/internal/api/client_retry_test.go new file mode 100644 index 0000000..bcced65 --- /dev/null +++ b/internal/api/client_retry_test.go @@ -0,0 +1,117 @@ +package api + +import ( + "context" + "errors" + "net" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" +) + +// TestLifecyclePOSTSendsContentLength guards the pause/resume regression. +// A body-less resty POST makes Go omit Content-Length, and control's +// forwarder drops any inbound content-length header, so the owning host +// answered "Content-Length is required" and every pause and resume failed +// while fork (which always had a body) kept working. +func TestLifecyclePOSTSendsContentLength(t *testing.T) { + for _, tc := range []struct { + name string + call func(*SandboxClient, context.Context) error + }{ + {"pause", func(c *SandboxClient, ctx context.Context) error { + _, err := c.PauseSandbox(ctx, "sb-1") + return err + }}, + {"resume", func(c *SandboxClient, ctx context.Context) error { + _, err := c.ResumeSandbox(ctx, "sb-1") + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + var gotLength int64 = -1 + var gotHeader, gotType string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotLength = r.ContentLength + gotHeader = r.Header.Get("Content-Length") + gotType = r.Header.Get("Content-Type") + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":{"id":"sb-1","status":"pausing"}}`)) + })) + defer srv.Close() + + client := NewSandboxClient("tok", srv.URL, false) + if err := tc.call(&client, context.Background()); err != nil { + t.Fatalf("call failed: %v", err) + } + if gotLength < 0 { + t.Errorf("ContentLength = %d, want >= 0 (unknown length means no header on the wire)", gotLength) + } + if gotHeader == "" { + t.Error("Content-Length header absent — this is the exact failure the fix targets") + } + if gotType != "application/json" { + t.Errorf("Content-Type = %q, want application/json (RequireJSON rejects anything else)", gotType) + } + }) + } +} + +// TestTransientRetryOnlyReplaysSafeRequests pins the split that keeps a +// retry from leaking a second billable sandbox: read-only methods retry on +// 5xx, mutating ones do not. +func TestTransientRetryOnlyReplaysSafeRequests(t *testing.T) { + for _, tc := range []struct { + name string + post bool + calls int32 + }{ + {"get retries on 500", false, transientRetryCount + 1}, + {"post does not retry on 500", true, 1}, + } { + t.Run(tc.name, func(t *testing.T) { + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&calls, 1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + client := NewSandboxClient("tok", srv.URL, false) + req := client.Client.R() + var err error + if tc.post { + _, err = req.SetBody(struct{}{}).Post("/v1/thing") + } else { + _, err = req.Get("/v1/thing") + } + if err != nil { + t.Fatalf("request error: %v", err) + } + if got := atomic.LoadInt32(&calls); got != tc.calls { + t.Errorf("server saw %d call(s), want %d", got, tc.calls) + } + }) + } +} + +func TestIsConnectSetupError(t *testing.T) { + for _, tc := range []struct { + name string + err error + want bool + }{ + {"dns failure never reached the server", &net.DNSError{Err: "no such host"}, true}, + {"dial timeout never reached the server", &net.OpError{Op: "dial", Err: errors.New("i/o timeout")}, true}, + {"read error means the request was already sent", &net.OpError{Op: "read", Err: errors.New("reset")}, false}, + {"write error means the request was already sent", &net.OpError{Op: "write", Err: errors.New("broken pipe")}, false}, + {"plain error", errors.New("boom"), false}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := isConnectSetupError(tc.err); got != tc.want { + t.Errorf("isConnectSetupError(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/internal/api/sandbox.go b/internal/api/sandbox.go index e129b75..feab790 100644 --- a/internal/api/sandbox.go +++ b/internal/api/sandbox.go @@ -393,13 +393,24 @@ func (c *SandboxClient) ForkSandbox(ctx context.Context, srcID string, req Sandb return &envelope.Data, nil } -// lifecyclePOST is the shared shape of pause/resume — body-less POST -// to /v1/sandboxes/{id}/, returning the updated view. +// lifecyclePOST is the shared shape of pause/resume — POST to +// /v1/sandboxes/{id}/, returning the updated view. +// +// The empty JSON body is load-bearing, not decoration. These actions carry +// no fields, but a resty request with no body at all makes Go omit +// Content-Length entirely, and control's forwarder drops any inbound +// content-length header before calling the owning host (see +// internal/control/handlers/forward.go in fc). The host then rejects the +// request with "Content-Length is required". ForkSandbox never hit this +// because it always had a body to send. Marshalling a struct also lets +// resty set Content-Type: application/json, which the RequireJSON +// middleware wants from any request that does carry a body. func (c *SandboxClient) lifecyclePOST(ctx context.Context, id, path string) (*SandboxView, error) { var envelope Response[SandboxView] resp, err := c.Client.R(). SetContext(ctx). SetPathParam("id", id). + SetBody(struct{}{}). SetResult(&envelope). Post(path) if err != nil { diff --git a/internal/api/sandbox_client.go b/internal/api/sandbox_client.go index 98c41a8..74d0fe8 100644 --- a/internal/api/sandbox_client.go +++ b/internal/api/sandbox_client.go @@ -67,6 +67,7 @@ func newSandboxClient(authHeader, token, sandboxURL string, debug bool, refreshe masked: maskToken(token), }) } + installTransientRetry(client) installAuthRefresh(client, authHeader, refresher) return SandboxClient{Client: client, authHeader: authHeader} } From 4fb5c4c186551d27e94f5e4b371cc3b8d12c29ae Mon Sep 17 00:00:00 2001 From: pratikbin <68642400+pratikbin@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:46:15 +0530 Subject: [PATCH 2/3] fix(sandbox): validate fork inputs and fail unknown commands fork --count was checked after resolving the source ref, so an invalid count still paid for an API lookup before failing. Move the check to the top of runFork. Unknown command names (root or nested) printed a suggestion but exited 0: urfave's CommandNotFoundFunc has no return value, so ShowCommandHelp always returns nil after calling it. Root's own custom Action also never reached that path at all, since urfave only falls back to CommandNotFoundFunc through its default help Action. Detect the unresolved name directly in each command's own Action instead, and return a real error so app.Run (and main.go's exit code) see the failure. Claude-Session: https://claude.ai/code/session_019J9PwFePTzAhpPs1TY9ZGq --- cmd/root/usage_error.go | 80 +++++++++++++++++++++++++++-------- cmd/root/usage_error_test.go | 79 ++++++++++++++++++++++++++++++++++ cmd/sandbox/fork.go | 7 +-- cmd/sandbox/lifecycle_test.go | 24 +++++++++++ 4 files changed, 169 insertions(+), 21 deletions(-) diff --git a/cmd/root/usage_error.go b/cmd/root/usage_error.go index 2098537..d9940c7 100644 --- a/cmd/root/usage_error.go +++ b/cmd/root/usage_error.go @@ -1,6 +1,7 @@ package root import ( + "errors" "fmt" "os" "strings" @@ -102,29 +103,72 @@ func correctedCommandLine(flagName string) string { } // installCommandSuggestions replaces urfave's bare "No help topic for -// 'ssh'" with the nearest real command. Agents and people both guess verb -// names, and a guess that lands one edit away from a real command should -// not cost a round trip to the help output. +// 'ssh'" with the nearest real command, and makes an unknown command name +// fail the run. Agents and people both guess verb names, and a guess that +// lands one edit away from a real command should not cost a round trip to +// the help output — and a guess that is simply wrong must not exit zero. +// +// This cannot be done with urfave's own CommandNotFoundFunc: ShowCommandHelp +// calls that callback and then unconditionally returns nil (see the +// package's help.go), so nothing set there can ever make app.Run return an +// error. Catching the unresolved name has to happen earlier, in the +// command's own Action, before urfave's help fallback runs. +// +// That is safe to do because an Action only ever runs with a positional +// argument still present when dispatch already failed to match that +// argument against a real subcommand — a match would have called that +// subcommand's Run instead. So "argument present here" and "unknown +// command" are the same condition. func installCommandSuggestions(app *cli.App) { - app.CommandNotFound = func(c *cli.Context, name string) { - // The hook fires for a subcommand too ("createos sandbox ssh"), and - // there the useful candidates are that group's subcommands, not the - // top-level verbs. Searching the wrong list is worse than staying - // quiet: it points at something unrelated. - candidates, prefix := app.Commands, "" - if cmd := c.Command; cmd != nil && len(cmd.Subcommands) > 0 { - candidates, prefix = cmd.Subcommands, cmd.Name+" " + fallback := app.Action + app.Action = func(c *cli.Context) error { + if name := c.Args().First(); name != "" { + return unknownCommandError(app.Commands, "", name) } - noun := "command" - if prefix != "" { - noun = "subcommand" + if fallback != nil { + return fallback(c) } - fmt.Fprintf(os.Stderr, "createos %s: %q is not a %s.\n", strings.TrimSpace(prefix), name, noun) - if best := nearestCommand(candidates, name); best != "" { - fmt.Fprintf(os.Stderr, "\n Did you mean:\n createos %s%s\n", prefix, best) + return cli.ShowSubcommandHelp(c) + } + for _, cmd := range app.Commands { + installGroupSuggestions(cmd) + } +} + +func installGroupSuggestions(cmd *cli.Command) { + if cmd == nil || len(cmd.Subcommands) == 0 { + return + } + prefix := cmd.Name + " " + fallback := cmd.Action + cmd.Action = func(c *cli.Context) error { + if name := c.Args().First(); name != "" { + return unknownCommandError(cmd.Subcommands, prefix, name) } - fmt.Fprintf(os.Stderr, "\n See everything with:\n createos %s--help\n", prefix) + if fallback != nil { + return fallback(c) + } + return cli.ShowSubcommandHelp(c) + } + for _, sub := range cmd.Subcommands { + installGroupSuggestions(sub) + } +} + +// unknownCommandError builds the same message the old CommandNotFound +// callback printed, but returns it instead of writing to stderr directly — +// main.go's error renderer prints whatever app.Run returns. +func unknownCommandError(candidates []*cli.Command, prefix, name string) error { + noun := "command" + if prefix != "" { + noun = "subcommand" + } + msg := fmt.Sprintf("createos %s: %q is not a %s.", strings.TrimSpace(prefix), name, noun) + if best := nearestCommand(candidates, name); best != "" { + msg += fmt.Sprintf("\n\n Did you mean:\n createos %s%s", prefix, best) } + msg += fmt.Sprintf("\n\n See everything with:\n createos %s--help", prefix) + return errors.New(msg) } // nearestCommand returns the closest command name within a small edit diff --git a/cmd/root/usage_error_test.go b/cmd/root/usage_error_test.go index 3e044a6..b6de1ab 100644 --- a/cmd/root/usage_error_test.go +++ b/cmd/root/usage_error_test.go @@ -3,6 +3,7 @@ package root import ( "errors" "os" + "strings" "testing" "github.com/urfave/cli/v2" @@ -95,6 +96,84 @@ func TestNearestCommand(t *testing.T) { } } +// newUnknownCommandTestApp builds a minimal command tree with the same +// shape as the real one (a root with subcommands, one of which is itself a +// group) and wires it through installCommandSuggestions exactly as +// root.NewApp does. It skips root.NewApp itself because that app's Before +// hook requires a signed-in session, which would make these tests depend on +// local auth state instead of on the routing bug being fixed. +func newUnknownCommandTestApp() *cli.App { + noop := func(_ *cli.Context) error { return nil } + app := &cli.App{ + Name: "createos", + Action: noop, // stands in for root.go's intro action + Commands: []*cli.Command{ + { + Name: "sandbox", + Subcommands: []*cli.Command{ + {Name: "offload", Action: noop}, + {Name: "list", Action: noop}, + }, + }, + {Name: "login", Action: noop}, + }, + } + installCommandSuggestions(app) + return app +} + +// TestUnknownCommandFailsTheRun pins the routing fix: urfave's own +// CommandNotFoundFunc can print a message but, per ShowCommandHelp in the +// library's help.go, can never make app.Run return an error — so both a +// root-level typo and a nested one used to print a suggestion and still +// exit 0. Every case here must return a non-nil error, since main.go's +// only success/failure signal is whether app.Run returned one. +func TestUnknownCommandFailsTheRun(t *testing.T) { + t.Run("root typo suggests the real command", func(t *testing.T) { + err := newUnknownCommandTestApp().Run([]string{"createos", "sandox"}) + if err == nil { + t.Fatal("want an error for an unknown top-level command") + } + if !strings.Contains(err.Error(), "createos sandbox") { + t.Errorf("error must suggest sandbox, got: %v", err) + } + }) + + t.Run("nested typo suggests the real subcommand", func(t *testing.T) { + err := newUnknownCommandTestApp().Run([]string{"createos", "sandbox", "ofload"}) + if err == nil { + t.Fatal("want an error for an unknown subcommand") + } + if !strings.Contains(err.Error(), "createos sandbox offload") { + t.Errorf("error must suggest sandbox offload, got: %v", err) + } + }) + + t.Run("gibberish gets an error but no false suggestion", func(t *testing.T) { + err := newUnknownCommandTestApp().Run([]string{"createos", "zzzzqqq"}) + if err == nil { + t.Fatal("want an error for an unknown top-level command") + } + if strings.Contains(err.Error(), "Did you mean") { + t.Errorf("must not guess a suggestion for gibberish input, got: %v", err) + } + }) +} + +// TestKnownCommandsStillDispatch guards against the fix being too broad: it +// must fail unresolved names, not every group invocation. +func TestKnownCommandsStillDispatch(t *testing.T) { + if err := newUnknownCommandTestApp().Run([]string{"createos", "sandbox", "list"}); err != nil { + t.Errorf("known subcommand must still dispatch normally, got: %v", err) + } + if err := newUnknownCommandTestApp().Run([]string{"createos", "sandbox"}); err != nil { + t.Errorf("a bare group with no subcommand must show help, not fail, got: %v", err) + } + if err := newUnknownCommandTestApp().Run([]string{"createos"}); err != nil { + t.Errorf("no arguments at all must still run the root action, got: %v", err) + } +} + func TestEditDistance(t *testing.T) { for _, tc := range []struct { a, b string diff --git a/cmd/sandbox/fork.go b/cmd/sandbox/fork.go index 04f7f6d..909c413 100644 --- a/cmd/sandbox/fork.go +++ b/cmd/sandbox/fork.go @@ -66,6 +66,10 @@ func runFork(c *cli.Context) error { return fmt.Errorf("you're not signed in — run 'createos login' to get started") } + if count := c.Int("count"); count < 1 { + return fmt.Errorf("--count must be at least 1 (got %d)", count) + } + ref := strings.TrimSpace(c.Args().First()) if ref == "" { if !terminal.IsInteractive() { @@ -90,9 +94,6 @@ func runFork(c *cli.Context) error { func runForkByID(c *cli.Context, client *api.SandboxClient, ref, srcID string) error { count := c.Int("count") - if count < 1 { - return fmt.Errorf("--count must be at least 1 (got %d)", count) - } req := api.SandboxForkReq{ StartPaused: c.Bool("paused"), diff --git a/cmd/sandbox/lifecycle_test.go b/cmd/sandbox/lifecycle_test.go index a90d698..2ada22d 100644 --- a/cmd/sandbox/lifecycle_test.go +++ b/cmd/sandbox/lifecycle_test.go @@ -303,6 +303,30 @@ func TestPauseForForkPausesASandboxTheCallerOwns(t *testing.T) { } } +// TestForkRejectsBadCountBeforeAnyAPICall covers the bug where --count was +// only checked after resolving the source ref, so `fork --count 0 missing` +// spent an API round trip on "missing" before ever complaining about the +// count. The count is knowable from the flags alone, so it must fail before +// any request goes out. +func TestForkRejectsBadCountBeforeAnyAPICall(t *testing.T) { + f := newFakeAPI(t) + app := &cli.App{ + Commands: []*cli.Command{newForkCommand()}, + Metadata: map[string]any{api.SandboxClientKey: f.client()}, + } + + err := app.RunContext(shortPoll(t), []string{"createos", "fork", "--count", "0", "missing"}) + if err == nil { + t.Fatal("want an error for --count 0") + } + if !strings.Contains(err.Error(), "--count must be at least 1 (got 0)") { + t.Errorf("error = %q, want it to name the bad count", err) + } + if len(f.seen) != 0 { + t.Errorf("count was invalid but the CLI still called the API: %v", f.seen) + } +} + // TestOffloadFailsWhenTeardownFails covers the leak that looks like a // clean run: the workload passes, DestroySandbox fails, and a CI job // reading only the exit status would never learn that a billable sandbox From 3b0a78c446e9a3c72621584f321b20102c3421d1 Mon Sep 17 00:00:00 2001 From: pratikbin <68642400+pratikbin@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:30:17 +0530 Subject: [PATCH 3/3] fix(sandbox): honor --count after sandbox ID Go's stdlib flag parsing stops at the first positional argument, so a command like `fork --count 2` left `--count` unparsed and defaulted to 1. Added `forkCountFlag` that falls back to scanning raw os.Args, restoring the intended count. --- cmd/sandbox/fork.go | 30 ++++++++++++++++++++++++-- cmd/sandbox/lifecycle_test.go | 40 +++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/cmd/sandbox/fork.go b/cmd/sandbox/fork.go index 909c413..1e9a426 100644 --- a/cmd/sandbox/fork.go +++ b/cmd/sandbox/fork.go @@ -3,6 +3,7 @@ package sandbox import ( "context" "fmt" + "strconv" "strings" "github.com/pterm/pterm" @@ -66,7 +67,7 @@ func runFork(c *cli.Context) error { return fmt.Errorf("you're not signed in — run 'createos login' to get started") } - if count := c.Int("count"); count < 1 { + if count := forkCountFlag(c); count < 1 { return fmt.Errorf("--count must be at least 1 (got %d)", count) } @@ -93,7 +94,7 @@ func runFork(c *cli.Context) error { } func runForkByID(c *cli.Context, client *api.SandboxClient, ref, srcID string) error { - count := c.Int("count") + count := forkCountFlag(c) req := api.SandboxForkReq{ StartPaused: c.Bool("paused"), @@ -300,3 +301,28 @@ type forkLeak struct { func (e *forkLeak) Error() string { return e.err.Error() } func (e *forkLeak) Unwrap() error { return e.err } + +// forkCountFlag reads --count the normal way, and falls back to a raw scan +// of os.Args when that comes back unset. +// +// Go's stdlib flag package, which urfave/cli sits on, stops parsing at the +// first non-flag argument. `fork --count 2` writes the sandbox +// first — the natural order — so `--count` is never parsed as a flag at +// all: it lands unread in c.Args(), and c.Int("count") silently returns +// the flag's default (1). No error, just the wrong count. This is the same +// shape of bug fixed for `process run --cwd` (commit 8c1f7ac); the +// fallback below reuses that fix's own raw-argv scanner. +func forkCountFlag(c *cli.Context) int { + if c.IsSet("count") { + return c.Int("count") + } + raw := rawProcessFlagValue("fork", "count") + if raw == "" { + return c.Int("count") + } + n, err := strconv.Atoi(raw) + if err != nil { + return c.Int("count") + } + return n +} diff --git a/cmd/sandbox/lifecycle_test.go b/cmd/sandbox/lifecycle_test.go index 2ada22d..990c5b0 100644 --- a/cmd/sandbox/lifecycle_test.go +++ b/cmd/sandbox/lifecycle_test.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "testing" "time" @@ -364,3 +365,42 @@ func TestOffloadFailsWhenTeardownFails(t *testing.T) { } } } + +// TestForkCountSurvivesTheNaturalArgumentOrder is the regression for a bug +// found live: `fork --count 2` — id first, the order every user +// and cos itself actually writes — silently forked once, not twice, with +// no error at all. Go's stdlib flag package stops parsing at the first +// non-flag argument, so `--count` written after the id is never parsed; +// c.Int("count") quietly returns the flag's default (1). The existing +// bad-count test only ever wrote --count before the id, so it never +// exercised this path. +func TestForkCountSurvivesTheNaturalArgumentOrder(t *testing.T) { + var forkCalls int32 + f := newFakeAPI(t). + json("GET /v1/sandboxes/sb-golden", `{"data":{"id":"sb-golden","status":"paused"}}`). + on("POST /v1/sandboxes/sb-golden/fork", func(w http.ResponseWriter, _ *http.Request) { + n := atomic.AddInt32(&forkCalls, 1) + fmt.Fprintf(w, `{"data":{"id":"sb-clone-%d","status":"running"}}`, n) + }). + json("GET /v1/sandboxes/sb-clone-1", `{"data":{"id":"sb-clone-1","status":"running"}}`). + json("GET /v1/sandboxes/sb-clone-2", `{"data":{"id":"sb-clone-2","status":"running"}}`) + + app := &cli.App{ + Commands: []*cli.Command{newForkCommand()}, + Metadata: map[string]any{api.SandboxClientKey: f.client()}, + } + + // The natural order: sandbox id first, --count after — exactly how cos + // and the fork.md example both write it. rawProcessFlagValue reads the + // real os.Args (that is the whole point — it recovers what urfave + // dropped), so the test has to set it, not just pass args to RunContext. + args := []string{"createos", "fork", "sb-golden", "--count", "2"} + withArgs(t, args) + if err := app.RunContext(shortPoll(t), args); err != nil { + t.Fatalf("fork: %v", err) + } + + if got := atomic.LoadInt32(&forkCalls); got != 2 { + t.Errorf("POST /fork called %d time(s), want 2 — --count 2 was silently dropped to 1", got) + } +}