diff --git a/free/ci/cmd/help_dispatch.go b/free/ci/cmd/help_dispatch.go new file mode 100644 index 000000000..00dd7c523 --- /dev/null +++ b/free/ci/cmd/help_dispatch.go @@ -0,0 +1,88 @@ +package main + +// Purpose: `nself ci --help` and `-h` print that subcommand's help and exit 0 before anything runs. +// Inputs: a matched key and the args after it; the v1.4 and v1.5 subcommand tables. +// Outputs: help text on stdout and exit code 0. +// Constraints: the dispatchers (run, runV15) call this before the handler and before the nodes +// --coordinator client, so a help request never starts a run or sends a request; a "--" terminator +// ends the scan (wantsHelp); a group word with no handler of its own (config, nodes pools) lists its +// children instead of falling through to the gate. + +import ( + "fmt" + "sort" + "strings" + + "github.com/nself-org/plugins/free/ci/internal/surface" +) + +// subHelp returns the help text of a registered key: the captured core text when there is one, else +// the usage line, the flag defaults of its V15 entry and the child commands under the key. +func subHelp(key string) string { + if txt, ok := surface.Help(key); ok { + return txt + } + var b strings.Builder + fmt.Fprintf(&b, "Usage: nself ci %s [flags]\n", key) + if e, ok := v15Subcommands[key]; ok && e.Flags != nil { + if fs := e.Flags(); fs != nil { + prev := fs.Output() + fs.SetOutput(&b) + b.WriteString("\nFlags:\n") + fs.PrintDefaults() + fs.SetOutput(prev) + } + } + if kids := childKeys(key); len(kids) > 0 { + b.WriteString("\nCommands:\n") + for _, k := range kids { + b.WriteString(" " + k + "\n") + } + } + return b.String() +} + +// childKeys lists the registered keys one or more tokens below key, sorted and without duplicates. +func childKeys(key string) []string { + seen := map[string]bool{} + prefix := key + " " + for k := range subcommands { + if strings.HasPrefix(k, prefix) { + seen[k] = true + } + } + for k, e := range v15Subcommands { + if strings.HasPrefix(k, prefix) && !e.Hidden { + seen[k] = true + } + } + out := make([]string, 0, len(seen)) + for k := range seen { + out = append(out, k) + } + sort.Strings(out) + return out +} + +// helpForGroup prints the child list of a leading word that names a group (for example "nodes pools") +// but no command of its own, and reports whether args named one. Only called when wantsHelp(args). +func helpForGroup(args []string) bool { + var words []string + for _, a := range args { + if strings.HasPrefix(a, "-") { + break + } + words = append(words, a) + } + for n := len(words); n >= 1; n-- { + key := strings.Join(words[:n], " ") + if kids := childKeys(key); len(kids) > 0 { + fmt.Print("Usage: nself ci " + key + " [flags]\n\nCommands:\n") + for _, k := range kids { + fmt.Println(" " + k) + } + return true + } + } + return false +} diff --git a/free/ci/cmd/help_dispatch_test.go b/free/ci/cmd/help_dispatch_test.go new file mode 100644 index 000000000..d36a6ff94 --- /dev/null +++ b/free/ci/cmd/help_dispatch_test.go @@ -0,0 +1,93 @@ +package main + +import ( + "strings" + "testing" +) + +// stubAllHandlers swaps every registered handler of both tables for one that records its key and +// returns 99, and restores the tables when the test ends. A help request that reaches a handler (or the +// gate or the bare run behind it) is then visible as a recorded key or a non-zero exit. +func stubAllHandlers(t *testing.T) *[]string { + t.Helper() + var ran []string + old14, old15 := subcommands, v15Subcommands + subcommands, v15Subcommands = map[string]handler{}, map[string]subcommandEntry{} + for k := range old14 { + key := "v14 " + k + subcommands[k] = func([]string) int { ran = append(ran, key); return 99 } + } + for k, e := range old15 { + key, entry := "v15 "+k, e + entry.Handler = func([]string) int { ran = append(ran, key); return 99 } + v15Subcommands[k] = entry + } + t.Cleanup(func() { subcommands, v15Subcommands = old14, old15 }) + return &ran +} + +// allKeys is every key of both tables, hidden entries included. +func allKeys() []string { + seen := map[string]bool{} + var out []string + for k := range subcommands { + seen[k] = true + } + for k := range v15Subcommands { + seen[k] = true + } + for k := range seen { + out = append(out, k) + } + return out +} + +// Asking any registered subcommand for help prints help, exits 0 and runs nothing: not the handler, not +// the nodes --coordinator client (which would dial the URL), not the bare run or the gate. The table is +// built from the registries, so a new subcommand is covered the day it is registered. +func TestEverySubcommandHelpRunsNothing(t *testing.T) { + keys := allKeys() + if len(keys) < 40 { + t.Fatalf("only %d registered keys; the registries did not load", len(keys)) + } + ran := stubAllHandlers(t) + t.Setenv("NSELF_CI_HOME", t.TempDir()) + t.Chdir(t.TempDir()) + for _, key := range keys { + for _, extra := range [][]string{ + {"--help"}, {"-h"}, {"-help"}, + {"--coordinator", "http://127.0.0.1:1", "--help"}, // must not dial + } { + args := append(strings.Fields(key), extra...) + for name, dispatch := range map[string]func([]string) int{"run": run, "runV15": runV15} { + var code int + out, errOut := captureStd(t, func() { code = dispatch(args) }) + if code != 0 || len(*ran) != 0 || strings.TrimSpace(out) == "" { + t.Errorf("%s %q: exit %d, handlers run %v, stdout %q, stderr %q; want exit 0, nothing run, help on stdout", + name, args, code, *ran, out, errOut) + *ran = nil + } + } + } + } +} + +// A help request after the "--" terminator belongs to the command being run, not to nself ci. +func TestHelpAfterTerminatorIsNotHelp(t *testing.T) { + if wantsHelp([]string{"--", "--help"}) || wantsHelp([]string{"-h", "--", "x"}) == false { + t.Fatal("wantsHelp must stop at the terminator and see a flag before it") + } +} + +// A group word with no command of its own lists its children and runs nothing. +func TestGroupHelpListsChildren(t *testing.T) { + ran := stubAllHandlers(t) + t.Chdir(t.TempDir()) + for group, child := range map[string]string{"config": "config show", "nodes pools": "nodes pools list", "policy": "policy show"} { + var code int + out, _ := captureStd(t, func() { code = runV15(append(strings.Fields(group), "--help")) }) + if code != 0 || len(*ran) != 0 || !strings.Contains(out, child) { + t.Errorf("%s --help: exit %d, ran %v, stdout %q; want exit 0, nothing run, %q listed", group, code, *ran, out, child) + } + } +} diff --git a/free/ci/cmd/main.go b/free/ci/cmd/main.go index 5b7fd8203..452b9deb1 100644 --- a/free/ci/cmd/main.go +++ b/free/ci/cmd/main.go @@ -43,21 +43,22 @@ func main() { os.Exit(run(os.Args[1:])) } -// run dispatches argv and returns the exit code. A bare -h/--help in front of -// a command that has captured core help prints that text; the first token -// that is not a flag selects a registered subcommand, otherwise the argv is -// the single-repo gate. +// run dispatches argv and returns the exit code. A -h/--help after a registered +// command prints that command's help and exits 0 without running it (help_dispatch.go). +// The first token that is not a flag selects a registered subcommand, otherwise +// the argv is the single-repo gate. func run(args []string) int { if h, key, rest, ok := resolve(args); ok { - if wantsHelp(rest) { - if printHelp(key) { - return 0 - } + if wantsHelp(rest) { // help never reaches the handler: it must not start anything + fmt.Print(subHelp(key)) + return 0 } return h(rest) } - if wantsHelp(args) && printHelp("") { - return 0 + if wantsHelp(args) { + if helpForGroup(args) || printHelp("") { + return 0 + } } return gateCmd(args) } diff --git a/free/ci/cmd/v15.go b/free/ci/cmd/v15.go index fda77934e..b4aeaa50c 100644 --- a/free/ci/cmd/v15.go +++ b/free/ci/cmd/v15.go @@ -96,6 +96,10 @@ func runV15(args []string) int { continue } if entry, ok := v15Subcommands[strings.Join(args[:n], " ")]; ok { + if wantsHelp(args[n:]) { // before the --coordinator client and the handler: help starts nothing + fmt.Print(subHelp(strings.Join(args[:n], " "))) + return 0 + } if code, done := nodesRemoteDispatch(strings.Join(args[:n], " "), args[n:]); done { // `nodes --coordinator ` (P7-NODE-26) return code } diff --git a/free/ci/internal/exec/copytree.go b/free/ci/internal/exec/copytree.go index 3396105eb..4da2b9527 100644 --- a/free/ci/internal/exec/copytree.go +++ b/free/ci/internal/exec/copytree.go @@ -49,11 +49,15 @@ func linkStaysInside(link, rel string) bool { return true } -func copyTree(source, dest string) error { +// copyTree copies source into dest. Paths in skip (and everything under them) are left out when they lie +// strictly inside source: the state directory and the workspace itself can sit in the repo root, and +// copying them would make the copy recurse into itself and hand the job other attempts' files. +func copyTree(source, dest string, skip ...string) error { root, err := filepath.Abs(source) if err != nil { return err } + skipped := skipRels(root, skip) return filepath.Walk(root, func(path string, info os.FileInfo, err error) error { if err != nil { return err @@ -65,6 +69,12 @@ func copyTree(source, dest string) error { if rel == "." { return nil } + if underAny(rel, skipped) { + if info.IsDir() { + return filepath.SkipDir + } + return nil + } target := filepath.Join(dest, rel) if info.Mode()&os.ModeSymlink != 0 { return copySymlink(path, rel, target) @@ -96,3 +106,44 @@ func copyTree(source, dest string) error { return closeErr }) } + +// skipRels turns the skip paths into paths relative to root, keeping only those strictly inside it. +// Root and each path are tried both as given and resolved through symlinks (a temp dir may be one on +// macOS), so the match does not depend on how the caller spelled them. +func skipRels(root string, skip []string) []string { + real := func(p string) string { + if r, err := filepath.EvalSymlinks(p); err == nil { + return r + } + return p + } + var out []string + for _, p := range skip { + if p == "" { + continue + } + abs, err := filepath.Abs(p) + if err != nil { + continue + } + for _, base := range []string{root, real(root)} { + for _, cand := range []string{abs, real(abs)} { + rel, err := filepath.Rel(base, cand) + if err == nil && rel != "." && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + out = append(out, rel) + } + } + } + } + return out +} + +// underAny reports whether rel is one of dirs or lies under one. +func underAny(rel string, dirs []string) bool { + for _, d := range dirs { + if rel == d || strings.HasPrefix(rel, d+string(filepath.Separator)) { + return true + } + } + return false +} diff --git a/free/ci/internal/exec/copytree_state_test.go b/free/ci/internal/exec/copytree_state_test.go new file mode 100644 index 000000000..1f20c6fe3 --- /dev/null +++ b/free/ci/internal/exec/copytree_state_test.go @@ -0,0 +1,104 @@ +package exec + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// writeTree creates the files (path -> content) under dir. +func writeTree(t *testing.T, dir string, files map[string]string) { + t.Helper() + for p, c := range files { + full := filepath.Join(dir, p) + if err := os.MkdirAll(filepath.Dir(full), 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(c), 0600); err != nil { + t.Fatal(err) + } + } +} + +// exists reports whether path is present (a link counts). +func exists(path string) bool { _, err := os.Lstat(path); return err == nil } + +// A skip path inside the source, and everything under it, is left out of the copy. Paths outside the +// source, the source itself and an ancestor of it change nothing. +func TestCopyTreeSkipsStatePaths(t *testing.T) { + src := t.TempDir() + writeTree(t, src, map[string]string{ + "a.txt": "a", "state/work/old/f.txt": "old", "state/db": "db", "statement.txt": "keep", "sub/b.txt": "b", + }) + outside := t.TempDir() + dst := filepath.Join(t.TempDir(), "ws") + if err := os.Mkdir(dst, 0700); err != nil { + t.Fatal(err) + } + // src itself, its parent and an unrelated dir are not skippable; "state" is. + if err := copyTree(src, dst, filepath.Join(src, "state"), src, filepath.Dir(src), outside, ""); err != nil { + t.Fatalf("copyTree: %v", err) + } + for _, p := range []string{"a.txt", "statement.txt", "sub/b.txt"} { + if !exists(filepath.Join(dst, p)) { + t.Errorf("%s missing from the copy", p) + } + } + if exists(filepath.Join(dst, "state")) { + t.Errorf("state dir was copied") + } +} + +// A symlinked spelling of the state dir (a temp dir is one on macOS) is matched too. +func TestCopyTreeSkipsStateThroughSymlink(t *testing.T) { + base := t.TempDir() + src := filepath.Join(base, "repo") + writeTree(t, src, map[string]string{"a.txt": "a", "state/x": "x"}) + alias := filepath.Join(base, "alias") + if err := os.Symlink(src, alias); err != nil { + t.Skipf("cannot create link: %v", err) + } + dst := filepath.Join(t.TempDir(), "ws") + if err := os.Mkdir(dst, 0700); err != nil { + t.Fatal(err) + } + if err := copyTree(src, dst, filepath.Join(alias, "state")); err != nil { + t.Fatalf("copyTree: %v", err) + } + if !exists(filepath.Join(dst, "a.txt")) || exists(filepath.Join(dst, "state")) { + t.Fatalf("want a.txt copied and state skipped") + } +} + +// With the state directory inside the repo root the workspace lands inside the source. The run must +// finish (no copy of the workspace into itself), the job must not see the state dir, and the repo files +// must be there, including a config file beside the state dir. +func TestRunWithStateDirInsideRepoRoot(t *testing.T) { + root := t.TempDir() + home := filepath.Join(root, ".nself", "ci") + writeTree(t, root, map[string]string{"README.md": "hi", ".nself/ci/work/older/leftover": "x", ".nself/ci/runs.db": "db", ".nself/ci.yaml": "cfg"}) + var out []byte + e := &Executor{Isolation: BaselineIsolator{}} + done := make(chan AttemptResult, 1) + go func() { + done <- e.Run(context.Background(), JobSpec{ + AttemptID: "inside", CoordinatorID: "coord", Home: home, SourceDir: root, + Command: []string{"sh", "-c", "ls -A .nself; test -f README.md && test -f .nself/ci.yaml && test ! -e .nself/ci"}, + Output: func(p []byte) { out = append(out, p...) }, + }) + }() + select { + case r := <-done: + if r.Err != nil || r.ExitCode != 0 { + t.Fatalf("run: %+v output %q", r, out) + } + if strings.TrimSpace(string(out)) != "ci.yaml" { + t.Fatalf(".nself listing = %q; want only the repo's ci.yaml, without the state dir", out) + } + case <-time.After(60 * time.Second): + t.Fatal("run did not finish: the workspace copy recursed into itself") + } +} diff --git a/free/ci/internal/exec/executor.go b/free/ci/internal/exec/executor.go index 145a05d76..ffcec14f9 100644 --- a/free/ci/internal/exec/executor.go +++ b/free/ci/internal/exec/executor.go @@ -164,8 +164,8 @@ func (e *Executor) Run(ctx context.Context, s JobSpec) (result AttemptResult) { return } defer func() { _ = pinned.Close() }() - if s.SourceDir != "" { - if err := copyTree(s.SourceDir, workspace); err != nil { + if s.SourceDir != "" { // the state dir and workspace may sit inside the repo root: they are not copied + if err := copyTree(s.SourceDir, workspace, s.Home, workspace); err != nil { result.Err = err return } diff --git a/free/ci/tools/doccmds/runner.go b/free/ci/tools/doccmds/runner.go index b0489ffa6..5579a34bc 100644 --- a/free/ci/tools/doccmds/runner.go +++ b/free/ci/tools/doccmds/runner.go @@ -23,16 +23,27 @@ func (p fixtureEnvPolicy) Environment(context.Context, model.Job) ([]string, err // runIn runs argv through the command funnel (internal/exec) with the fixture environment of dir. // The command starts in the executor's empty workspace; out receives its output. +// +// timeout is a wall-clock bound carried by the context, not by JobSpec.Timeout: the funnel turns a +// JobSpec timeout into a CPU ulimit (hard limit), and a block that runs `nself ci -- cmd` starts an +// inner job whose own limit (30m by default) is then above the block's. The inner job cannot raise +// its hard limit and dies with "ulimit: Operation not permitted". Leaving the block without a CPU +// limit lets the inner job set its own, and the context deadline still fails a hung block. func runIn(ctx context.Context, dir string, timeout time.Duration, out func([]byte), argv ...string) (ciexec.AttemptResult, error) { home, err := os.MkdirTemp("", "doccmds-home-") if err != nil { return ciexec.AttemptResult{}, err } defer func() { _ = os.RemoveAll(home) }() + if timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } ex := ciexec.Executor{Isolation: ciexec.BaselineIsolator{}, Env: fixtureEnvPolicy{dir}} res := ex.Run(ctx, ciexec.JobSpec{ AttemptID: "doccmds-" + strconv.FormatInt(time.Now().UnixNano(), 36), CoordinatorID: "doccmds", - Job: model.Job{Kind: "static"}, Home: home, Timeout: timeout, Command: argv, Output: out, + Job: model.Job{Kind: "static"}, Home: home, Command: argv, Output: out, }) return res, nil } diff --git a/free/ci/tools/doccmds/runner_test.go b/free/ci/tools/doccmds/runner_test.go new file mode 100644 index 000000000..17f736837 --- /dev/null +++ b/free/ci/tools/doccmds/runner_test.go @@ -0,0 +1,41 @@ +package main + +import ( + "strings" + "testing" + "time" +) + +// A block that starts a job with a CPU limit of its own must not be refused because the block itself +// carries a lower one: an ad hoc `nself ci -- cmd` example runs a 30 minute inner job inside a block with a +// 10 minute bound. The inner `ulimit -t` raises the hard limit above the block's and fails with +// "Operation not permitted" when the block was started under a CPU ulimit. +func TestBlockCanStartJobWithHigherCPULimit(t *testing.T) { + dir, cleanup, err := newFixture() + if err != nil { + t.Fatal(err) + } + defer cleanup() + r := shellRunner{timeout: time.Minute} + code, err := r.Run(dir, "sh -c 'ulimit -t 1800 || exit 42; echo inner-ok'") + if err != nil || code != 0 { + t.Fatalf("block exit %d, err %v; want 0 (the inner job sets a limit above the block's bound)", code, err) + } +} + +// The wall-clock bound still fails a hung block, and the error names the bound. +func TestBlockTimeoutStillFailsHungBlock(t *testing.T) { + dir, cleanup, err := newFixture() + if err != nil { + t.Fatal(err) + } + defer cleanup() + start := time.Now() + code, err := shellRunner{timeout: 500 * time.Millisecond}.Run(dir, "sleep 30") + if err == nil || code != -1 || !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("exit %d, err %v; want -1 and a timeout error", code, err) + } + if time.Since(start) > 20*time.Second { + t.Fatalf("a hung block ran %s past its bound", time.Since(start)) + } +}