diff --git a/.github/command-inventory.json b/.github/command-inventory.json index 949b2139..6cf9795a 100644 --- a/.github/command-inventory.json +++ b/.github/command-inventory.json @@ -371,6 +371,7 @@ "--no-monorepo", "--profile", "--quiet", + "--remove-orphans", "--security-report", "--verbose" ] diff --git a/.github/wiki/cmd-build.md b/.github/wiki/cmd-build.md index c5df28c5..e4f85c2b 100644 --- a/.github/wiki/cmd-build.md +++ b/.github/wiki/cmd-build.md @@ -106,6 +106,7 @@ Each long-running service gets a default pids limit of 100 to prevent fork-bomb | `--no-monorepo` | `false` | Disable automatic monorepo backend detection | | `--profile` | `""` | Service profile: curated subset of services to include in docker-compose.yml. app (default) — full service set, identical to pre-profile behaviour. ops — observability + CI server: postgres, hasura, auth, nginx, monitoring stack; excludes minio, mailpit, admin, functions, search. Overrides NSELF_PROFILE env var. Valid values: app, ops. | | `--quiet`, `-q` | `false` | Suppress non-error output (for CI use) | +| `--remove-orphans` | `false` | Remove containers with no matching service in the freshly generated compose (G-014). Detection always runs; removal is opt-in. | | `--security-report` | `false` | Generate security analysis | | `--verbose`, `-v` | `false` | Show environment cascade | | `--help`, `-h` | — | Show help | diff --git a/.github/wiki/llms.txt b/.github/wiki/llms.txt index da988aa3..80f0831f 100644 --- a/.github/wiki/llms.txt +++ b/.github/wiki/llms.txt @@ -123,6 +123,7 @@ Flags: monitoring stack; excludes minio, mailpit, admin, functions, search. Overrides NSELF_PROFILE env var. Valid values: app, ops. - `--quiet` (default false) — Suppress non-error output (for CI use) +- `--remove-orphans` (default false) — Remove containers with no matching service in the freshly generated compose (G-014). Detection always runs; removal is opt-in. - `--security-report` (default false) — Generate security analysis - `--verbose` (default false) — Show environment cascade diff --git a/cmd/commands/build.go b/cmd/commands/build.go index a5088e05..708752f7 100644 --- a/cmd/commands/build.go +++ b/cmd/commands/build.go @@ -1,18 +1,15 @@ package commands import ( - "context" "fmt" "os" "path/filepath" "strings" - "time" "github.com/nself-org/cli/internal/build" "github.com/nself-org/cli/internal/compose" "github.com/nself-org/cli/internal/config" "github.com/nself-org/cli/internal/migration" - "github.com/nself-org/cli/internal/plugin" "github.com/nself-org/cli/internal/ui" "github.com/spf13/cobra" @@ -45,6 +42,7 @@ func init() { buildCmd.Flags().Bool("no-migration-check", false, "Skip v1 artifact detection (for automation/CI)") buildCmd.Flags().Bool("allow-legacy", false, "Bypass v0.9 artifact check and proceed with WARNING (not recommended)") buildCmd.Flags().Bool("no-auto-redis", false, "Disable automatic Redis enablement when a BullMQ-backed plugin is detected") + buildCmd.Flags().Bool("remove-orphans", false, "Remove containers with no matching service in the freshly generated compose (G-014). Detection always runs; removal is opt-in.") buildCmd.Flags().String("profile", "", `Service profile: curated subset of services to include in docker-compose.yml. app (default) — full service set, identical to pre-profile behaviour. ops — observability + CI server: postgres, hasura, auth, nginx, @@ -67,6 +65,7 @@ func runBuild(cmd *cobra.Command, args []string) error { noMigrationCheck, _ := cmd.Flags().GetBool("no-migration-check") allowLegacy, _ := cmd.Flags().GetBool("allow-legacy") noAutoRedis, _ := cmd.Flags().GetBool("no-auto-redis") + removeOrphans, _ := cmd.Flags().GetBool("remove-orphans") // ── Profile resolution ──────────────────────────────────────────── // Priority: --profile flag > NSELF_PROFILE env var > default ("app"). @@ -227,73 +226,12 @@ func runBuild(cmd *cobra.Command, args []string) error { ui.Info("Next step: nself start") } - return nil -} - -// runPluginLifecycleCheck loads the lifecycle store, transitions expired plugins, -// prints dormant banners, and auto-removes fully-expired plugins. -// Auto-removal is intentionally build-only (not start) — start is read-only on lifecycle. -func runPluginLifecycleCheck(quiet bool) { - store, err := plugin.LoadLifecycleStore() - if err != nil { - // Non-fatal: lifecycle store is advisory only. - if !quiet { - ui.Warn("Could not load plugin lifecycle store: " + err.Error()) - } - return + // ── G-014: orphan container detection (always on) + removal (opt-in + // via --remove-orphans) — see build_orphans.go. Never runs for --check, + // which returns before ComposeFile is generated. + if !check && result.ComposeFile != "" { + reportAndHandleOrphans(workdir, result, removeOrphans, quiet) } - now := time.Now() - dormant, autoRemove := store.CheckExpiry(now) - - // Print dormant banners. - for _, name := range dormant { - if rec, ok := store.Records[name]; ok && !quiet { - ui.Warn(plugin.DormantBanner(rec, now)) - } - } - - // Print banners for already-dormant plugins (transitioned in a prior run). - for name, rec := range store.Records { - if rec.State == plugin.StateDormant { - alreadyPrinted := false - for _, d := range dormant { - if d == name { - alreadyPrinted = true - break - } - } - if !alreadyPrinted && !quiet { - ui.Warn(plugin.DormantBanner(rec, now)) - } - } - } - - // Auto-remove expired plugins. - for _, name := range autoRemove { - if !quiet { - ui.Warn(fmt.Sprintf("Removing expired plugin %q (grace period exhausted)", name)) - } - cfg, cfgErr := config.Load(".") - if cfgErr != nil { - // Fall back to default plugin dir. - cfg = &config.Config{} - } - pluginDir := resolvePluginDir() - if removeErr := plugin.Remove(context.Background(), cfg, name, pluginDir, false, true); removeErr != nil { - if !quiet { - ui.Warn(fmt.Sprintf("Auto-remove of %q failed: %v", name, removeErr)) - } - } else { - // Clear the record after successful removal. - delete(store.Records, name) - } - } - - // Persist transitions (dormant → expired state changes). - if len(dormant) > 0 || len(autoRemove) > 0 { - if saveErr := store.Save(); saveErr != nil && !quiet { - ui.Warn("Could not save plugin lifecycle store: " + saveErr.Error()) - } - } + return nil } diff --git a/cmd/commands/build_orphans.go b/cmd/commands/build_orphans.go new file mode 100644 index 00000000..beaf9d5f --- /dev/null +++ b/cmd/commands/build_orphans.go @@ -0,0 +1,132 @@ +package commands + +// build_orphans.go — G-014: `nself build` orphan-container detection +// (always on) and removal (opt-in via --remove-orphans). +// +// WHY: nself build only ever generated files; it never looked at the live +// daemon, so a service dropped from the generated compose (removed from +// nself.yaml, an uninstalled plugin, a rename) left its container running +// forever with no service definition, no nginx vhost, and no traffic — +// unreported. Measured live: nself-claw/notify/mux/cron survived an earlier +// build this way; two had been dead a full month (DB DNS failure) before +// anyone noticed, because a second bug made their healthchecks permanently +// meaningless too (see internal/doctor/deep_docker_healthcheck.go). +// +// Purpose: report every such container prominently after every build, and +// remove them only when the operator opted in. +// Inputs: workdir (the project root just built), result (the BuildResult +// from build.Build), removeOrphans (--remove-orphans), quiet. +// Outputs: none — prints to stdout/stderr; never returns an error, because +// orphan detection is advisory and must never fail `nself build` +// itself (a Docker-less CI image must still get a successful build). +// Constraints: project-scoped via internal/docker.DetectOrphans's compose +// project label filter (see that file's scoping note) — never +// touches a container from another project or a non-compose +// container. + +import ( + "context" + "fmt" + "log/slog" + "os" + "time" + + "github.com/nself-org/cli/internal/build" + "github.com/nself-org/cli/internal/docker" + "github.com/nself-org/cli/internal/ui" +) + +// detectOrphansForProject resolves the effective compose file set for +// workdir (base + plugin fragments, falling back to fallbackComposeFile +// when the manifest is missing) and returns the containers orphaned against +// it. Shared by `nself build` (reportAndHandleOrphans, below) and `nself +// status` (see status.go), which both need the same detection but differ on +// what to do with the result — build offers --remove-orphans, status is +// read-only. Returns (nil, nil) whenever detection is inconclusive (compose +// files unreadable, docker unreachable); callers must treat that as "nothing +// to report", never as a hard failure of their own command. +func detectOrphansForProject(ctx context.Context, workdir, projectName, fallbackComposeFile string) []docker.OrphanContainer { + composeFiles, err := build.ReadComposeManifest(workdir) + if err != nil || len(composeFiles) == 0 { + composeFiles = []string{fallbackComposeFile} + } + + defined, err := docker.ComposeServiceNames(composeFiles) + if err != nil { + slog.Debug("G-014 orphan detection: could not read compose files", "err", err) + return nil + } + + orphans, err := docker.DetectOrphans(ctx, projectName, defined) + if err != nil { + slog.Debug("G-014 orphan detection: docker unavailable, skipping", "err", err) + return nil + } + return orphans +} + +// reportAndHandleOrphans runs orphan detection for the project just built +// and, if --remove-orphans was passed, removes what it finds. Best-effort +// throughout: any docker-level failure (daemon unreachable, docker not +// installed) is logged at debug level and swallowed so `nself build` itself +// never fails because of it. +func reportAndHandleOrphans(workdir string, result *build.BuildResult, removeOrphans, quiet bool) { + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + orphans := detectOrphansForProject(ctx, workdir, result.ProjectName, result.ComposeFile) + if len(orphans) == 0 { + return + } + + if !quiet { + printOrphanWarning(orphans) + } + + if !removeOrphans { + if !quiet { + ui.Warn(fmt.Sprintf("Run 'nself build --remove-orphans' to remove the %d container(s) above.", len(orphans))) + } + return + } + + removeErrs := docker.RemoveOrphans(ctx, orphans) + if quiet { + return + } + removed := len(orphans) - len(removeErrs) + if removed > 0 { + ui.Success(fmt.Sprintf("Removed %d orphaned container(s)", removed)) + } + for _, e := range removeErrs { + ui.Warn(e.Error()) + } +} + +// printOrphanWarning prints the prominent, always-on orphan report: one +// line per container so an operator scanning build output cannot miss it. +func printOrphanWarning(orphans []docker.OrphanContainer) { + ui.Warn(fmt.Sprintf("%d orphaned container(s) found — running but with no matching service in the generated compose:", len(orphans))) + for _, o := range orphans { + service := o.Service + if service == "" { + service = "(none)" + } + fmt.Fprintf(os.Stderr, " %-30s service=%-12s state=%s\n", o.Name, service, o.State) + } +} + +// printOrphanStatusHint runs the same G-014 orphan detection for `nself +// status` (read-only — status never removes anything; that stays build's +// --remove-orphans opt-in) and, if any are found, prints a warning block +// pointing at the fix. Best-effort and silent on any docker-level failure — +// status must never fail or slow down because of this. +func printOrphanStatusHint(ctx context.Context, workdir, projectName, composeFile string) { + orphans := detectOrphansForProject(ctx, workdir, projectName, composeFile) + if len(orphans) == 0 { + return + } + fmt.Println() + printOrphanWarning(orphans) + ui.Warn("Run 'nself build --remove-orphans' to remove them.") +} diff --git a/cmd/commands/build_plugin_lifecycle.go b/cmd/commands/build_plugin_lifecycle.go new file mode 100644 index 00000000..062d90a9 --- /dev/null +++ b/cmd/commands/build_plugin_lifecycle.go @@ -0,0 +1,84 @@ +package commands + +// build_plugin_lifecycle.go — plugin dormant/expiry banner + auto-removal +// for `nself build`. Split out of build.go (kept build.go under the +// 300-line cap when G-014 orphan-container detection was added) as a pure +// move: same checks/output/errors/order, no behavior change. + +import ( + "context" + "fmt" + "time" + + "github.com/nself-org/cli/internal/config" + "github.com/nself-org/cli/internal/plugin" + "github.com/nself-org/cli/internal/ui" +) + +// runPluginLifecycleCheck loads the lifecycle store, transitions expired plugins, +// prints dormant banners, and auto-removes fully-expired plugins. +// Auto-removal is intentionally build-only (not start) — start is read-only on lifecycle. +func runPluginLifecycleCheck(quiet bool) { + store, err := plugin.LoadLifecycleStore() + if err != nil { + // Non-fatal: lifecycle store is advisory only. + if !quiet { + ui.Warn("Could not load plugin lifecycle store: " + err.Error()) + } + return + } + + now := time.Now() + dormant, autoRemove := store.CheckExpiry(now) + + // Print dormant banners. + for _, name := range dormant { + if rec, ok := store.Records[name]; ok && !quiet { + ui.Warn(plugin.DormantBanner(rec, now)) + } + } + + // Print banners for already-dormant plugins (transitioned in a prior run). + for name, rec := range store.Records { + if rec.State == plugin.StateDormant { + alreadyPrinted := false + for _, d := range dormant { + if d == name { + alreadyPrinted = true + break + } + } + if !alreadyPrinted && !quiet { + ui.Warn(plugin.DormantBanner(rec, now)) + } + } + } + + // Auto-remove expired plugins. + for _, name := range autoRemove { + if !quiet { + ui.Warn(fmt.Sprintf("Removing expired plugin %q (grace period exhausted)", name)) + } + cfg, cfgErr := config.Load(".") + if cfgErr != nil { + // Fall back to default plugin dir. + cfg = &config.Config{} + } + pluginDir := resolvePluginDir() + if removeErr := plugin.Remove(context.Background(), cfg, name, pluginDir, false, true); removeErr != nil { + if !quiet { + ui.Warn(fmt.Sprintf("Auto-remove of %q failed: %v", name, removeErr)) + } + } else { + // Clear the record after successful removal. + delete(store.Records, name) + } + } + + // Persist transitions (dormant → expired state changes). + if len(dormant) > 0 || len(autoRemove) > 0 { + if saveErr := store.Save(); saveErr != nil && !quiet { + ui.Warn("Could not save plugin lifecycle store: " + saveErr.Error()) + } + } +} diff --git a/cmd/commands/status.go b/cmd/commands/status.go index a244aafa..f4a40d58 100644 --- a/cmd/commands/status.go +++ b/cmd/commands/status.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "path/filepath" "time" "github.com/nself-org/cli/internal/build" @@ -101,6 +102,13 @@ Exit codes: printStatusTable(report, verbose, healthOnly, metrics) + // G-014: surface orphaned containers (running with no matching + // service in the current compose) here too, not just at build time — + // a container can go orphaned between builds if someone removes it + // from nself.yaml and never reruns `nself build`. Read-only: status + // never removes anything itself. + printOrphanStatusHint(ctx, cwd, cfg.ProjectName, filepath.Join(cwd, "docker-compose.yml")) + // Show installed plugin versions below the service health table. pluginDir := build.DefaultPluginDir() if plugins, pluginErr := plugin.ListInstalled(pluginDir); pluginErr == nil && len(plugins) > 0 { diff --git a/internal/docker/orphans.go b/internal/docker/orphans.go new file mode 100644 index 00000000..e96d4e43 --- /dev/null +++ b/internal/docker/orphans.go @@ -0,0 +1,181 @@ +package docker + +// orphans.go — detection and (opt-in) removal of "orphaned" containers: ones +// that were created by a previous `nself build` + `docker compose up` for +// THIS project but whose service definition no longer exists in the +// freshly generated compose files. +// +// WHY (CLI gap G-014): a service dropped from docker-compose.yml (a service +// removed from nself.yaml, a plugin uninstalled, a renamed service) leaves +// its container running forever — `nself build` never looked at the live +// daemon at all, so nothing noticed. Measured live on prod: four containers +// (nself-claw, nself-notify, nself-mux, nself-cron) had no service +// definition, no nginx vhost, and no traffic; two of them had been dead +// since a database DNS failure a MONTH earlier and nobody noticed, because +// their healthchecks could never report anything meaningful either (see +// deep_docker.go's healthcheck-binary check for the other half of that +// incident). Detection must be on by default; removal must be opt-in, +// because an operator may have started a container by hand for debugging +// and not want it silently reaped. +// +// PROJECT SCOPING (read this before changing the filter below): every +// container docker compose creates is labeled com.docker.compose.project= +// , where is exactly the compose file's top-level `name:` +// field — which nSelf sets to cfg.ProjectName (see internal/compose +// Generator.buildDockerCompose). DetectOrphans filters `docker ps` on that +// exact label before it ever looks at service names, so a container from a +// different project (different label value) or a container docker didn't +// create via compose (no label at all) can never be selected — the filter +// runs server-side in the docker CLI, not as a client-side name guess. +// Never widen this to a bare `docker ps -a` scan. + +import ( + "bufio" + "context" + "fmt" + "os" + "os/exec" + "strings" + + "gopkg.in/yaml.v3" +) + +// composeProjectLabel is the label Docker Compose stamps on every container +// it creates, set to the compose file's top-level `name:` (nSelf's +// cfg.ProjectName). Filtering on it is what makes orphan detection safe to +// run against a host with other, unrelated Docker workloads. +const composeProjectLabel = "com.docker.compose.project" + +// composeServiceLabel is the label holding the service name a container was +// created for. Compared against the freshly generated compose files' service +// set to decide whether a container is an orphan. +const composeServiceLabel = "com.docker.compose.service" + +// OrphanContainer describes a running-or-stopped container that belongs to +// this project (by compose project label) but has no matching service in +// the freshly generated compose files. +type OrphanContainer struct { + ID string + Name string + Service string // may be empty if the container somehow lacks the label + State string +} + +// minimalComposeFile mirrors only the piece of a docker-compose.yml this +// package needs: the set of service names. Deliberately not compose.DockerCompose +// (internal/compose) — plugin-authored compose fragments are arbitrary, +// hand-written YAML and must not be forced through the stricter generator +// struct just to read their top-level keys. +type minimalComposeFile struct { + Services map[string]yaml.Node `yaml:"services"` +} + +// ComposeServiceNames reads every compose file in composeFilePaths (the base +// docker-compose.yml plus any plugin compose fragments — the same file set +// `docker compose -f ... -f ...` is invoked with, see +// build.ReadComposeManifest) and returns the union of all service names they +// define. This is the "desired state" DetectOrphans compares live containers +// against. +// +// Inputs: composeFilePaths — absolute paths to YAML files; missing files are +// +// skipped (best-effort — a plugin fragment can be removed from disk +// independently of the manifest that references it). +// +// Outputs: the union of service keys across all readable files, and the +// +// first hard parse error encountered (a file that exists but is not +// valid YAML is a real problem, not a missing-file gap). +// +// Constraints: pure I/O + YAML parsing, no docker daemon access. +func ComposeServiceNames(composeFilePaths []string) (map[string]struct{}, error) { + names := make(map[string]struct{}) + for _, path := range composeFilePaths { + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + continue + } + return nil, fmt.Errorf("reading compose file %s: %w", path, err) + } + var doc minimalComposeFile + if err := yaml.Unmarshal(data, &doc); err != nil { + return nil, fmt.Errorf("parsing compose file %s: %w", path, err) + } + for name := range doc.Services { + names[name] = struct{}{} + } + } + return names, nil +} + +// buildOrphanPsArgs returns the `docker ps` arguments that list every +// container (running or stopped) carrying this project's compose-project +// label, one tab-separated line per container: ID, Name, Service label, +// State. Separated from DetectOrphans so the exact filter/format can be +// pinned by a test without a live daemon. +func buildOrphanPsArgs(projectName string) []string { + return []string{ + "ps", "-a", + "--filter", fmt.Sprintf("label=%s=%s", composeProjectLabel, projectName), + "--format", fmt.Sprintf(`{{.ID}}\t{{.Names}}\t{{.Label %q}}\t{{.State}}`, composeServiceLabel), + } +} + +// parseOrphanPsOutput parses buildOrphanPsArgs' tab-separated output and +// returns the containers whose service label is not in defined. A container +// with a blank service label (should not normally happen for a +// compose-created container, but never assume) is treated as an orphan too — +// reporting an unexpected container is always safer than silently ignoring +// it. +func parseOrphanPsOutput(raw string, defined map[string]struct{}) []OrphanContainer { + var orphans []OrphanContainer + scanner := bufio.NewScanner(strings.NewReader(raw)) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue + } + parts := strings.Split(line, "\t") + if len(parts) < 4 { + continue + } + id, name, service, state := parts[0], parts[1], parts[2], parts[3] + if _, ok := defined[service]; ok { + continue + } + orphans = append(orphans, OrphanContainer{ID: id, Name: name, Service: service, State: state}) + } + return orphans +} + +// DetectOrphans lists every container belonging to projectName (by compose +// project label — see the package-level scoping note above) and returns the +// ones whose service is not present in defined. Returns an error only when +// the docker CLI itself could not be run (daemon unreachable, binary +// missing); callers should treat that as advisory-only and skip reporting +// rather than failing the caller's own command (a `nself build` run in a +// docker-less CI image must still succeed). +func DetectOrphans(ctx context.Context, projectName string, defined map[string]struct{}) ([]OrphanContainer, error) { + cmd := exec.CommandContext(ctx, "docker", buildOrphanPsArgs(projectName)...) + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("docker ps (project=%s): %w", projectName, err) + } + return parseOrphanPsOutput(string(out), defined), nil +} + +// RemoveOrphans force-removes each orphan container by ID (docker rm -f — +// same primitive cleanup.go's forceRemoveContainer uses for init/zombie +// containers) and returns one error per failed removal. Never called unless +// the caller opted in (e.g. `nself build --remove-orphans`); detection alone +// never removes anything. +func RemoveOrphans(ctx context.Context, orphans []OrphanContainer) []error { + var errs []error + for _, o := range orphans { + if err := forceRemoveContainer(ctx, o.ID); err != nil { + errs = append(errs, fmt.Errorf("removing orphan container %s (%s): %w", o.Name, o.Service, err)) + } + } + return errs +} diff --git a/internal/docker/orphans_test.go b/internal/docker/orphans_test.go new file mode 100644 index 00000000..4816fa94 --- /dev/null +++ b/internal/docker/orphans_test.go @@ -0,0 +1,136 @@ +package docker + +// Tests for G-014 orphan detection. No live docker daemon or real project +// containers required: buildOrphanPsArgs and parseOrphanPsOutput are pure +// functions exercised directly with canned `docker ps` output, and +// ComposeServiceNames is exercised against temp YAML files on disk. + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestBuildOrphanPsArgs_ScopesToProjectLabel(t *testing.T) { + args := buildOrphanPsArgs("myproj") + joined := strings.Join(args, " ") + + if !strings.Contains(joined, "label=com.docker.compose.project=myproj") { + t.Errorf("args %q must filter on the project label, so unrelated containers on the host are never selected", joined) + } + if !strings.Contains(joined, `-a`) { + t.Errorf("args %q must include -a so stopped orphans (e.g. a dead-since-last-month container) are found too", joined) + } + if !strings.Contains(joined, "com.docker.compose.service") { + t.Errorf("args %q must request the service label so orphan detection can compare it", joined) + } +} + +func TestParseOrphanPsOutput_FlagsUndefinedServices(t *testing.T) { + // Mirrors the measured prod incident: nself-claw/notify/mux/cron have a + // project label but no matching service in the freshly generated compose. + raw := strings.Join([]string{ + "abc123\tnself_postgres\tpostgres\trunning", + "def456\tnself-claw\tclaw\trunning", + "ghi789\tnself-mux\tmux\texited", + }, "\n") + defined := map[string]struct{}{"postgres": {}, "hasura": {}} + + orphans := parseOrphanPsOutput(raw, defined) + if len(orphans) != 2 { + t.Fatalf("expected 2 orphans, got %d: %+v", len(orphans), orphans) + } + names := map[string]bool{orphans[0].Name: true, orphans[1].Name: true} + if !names["nself-claw"] || !names["nself-mux"] { + t.Errorf("expected nself-claw and nself-mux flagged as orphans, got %+v", orphans) + } + for _, o := range orphans { + if o.Service == "postgres" { + t.Errorf("postgres has a matching service definition and must never be flagged: %+v", o) + } + } +} + +func TestParseOrphanPsOutput_NoOrphansWhenAllDefined(t *testing.T) { + raw := "abc123\tnself_postgres\tpostgres\trunning\ndef456\tnself_hasura\thasura\trunning" + defined := map[string]struct{}{"postgres": {}, "hasura": {}} + + orphans := parseOrphanPsOutput(raw, defined) + if len(orphans) != 0 { + t.Errorf("expected no orphans when every container's service is defined, got %+v", orphans) + } +} + +func TestParseOrphanPsOutput_BlankServiceLabelIsFlagged(t *testing.T) { + // A container with a compose-project label but no service label is + // unexpected; report it rather than silently skip it. + raw := "abc123\tweird-container\t\trunning" + orphans := parseOrphanPsOutput(raw, map[string]struct{}{"postgres": {}}) + if len(orphans) != 1 { + t.Fatalf("expected the blank-service container to be flagged, got %d orphans", len(orphans)) + } +} + +func TestParseOrphanPsOutput_EmptyAndMalformedLinesSkipped(t *testing.T) { + raw := "\n\nabc123\tonlytwo\tfields\n" + orphans := parseOrphanPsOutput(raw, map[string]struct{}{}) + if len(orphans) != 0 { + t.Errorf("malformed (too few columns) and blank lines must be skipped, got %+v", orphans) + } +} + +func TestComposeServiceNames_UnionsMultipleFiles(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "docker-compose.yml") + plugin := filepath.Join(dir, "plugin-claw.yml") + + if err := os.WriteFile(base, []byte("name: myproj\nservices:\n postgres:\n image: postgres:16\n hasura:\n image: hasura/graphql-engine\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(plugin, []byte("services:\n claw:\n image: nself/claw:latest\n"), 0o644); err != nil { + t.Fatal(err) + } + + names, err := ComposeServiceNames([]string{base, plugin}) + if err != nil { + t.Fatalf("ComposeServiceNames returned error: %v", err) + } + for _, want := range []string{"postgres", "hasura", "claw"} { + if _, ok := names[want]; !ok { + t.Errorf("expected service %q in union, got %+v", want, names) + } + } + if len(names) != 3 { + t.Errorf("expected exactly 3 services, got %d: %+v", len(names), names) + } +} + +func TestComposeServiceNames_MissingFileSkippedNotFatal(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "docker-compose.yml") + if err := os.WriteFile(base, []byte("services:\n postgres:\n image: postgres:16\n"), 0o644); err != nil { + t.Fatal(err) + } + missing := filepath.Join(dir, "removed-plugin.yml") + + names, err := ComposeServiceNames([]string{base, missing}) + if err != nil { + t.Fatalf("a missing compose fragment must not be a hard error, got: %v", err) + } + if _, ok := names["postgres"]; !ok { + t.Errorf("expected postgres from the readable file, got %+v", names) + } +} + +func TestComposeServiceNames_InvalidYAMLIsHardError(t *testing.T) { + dir := t.TempDir() + bad := filepath.Join(dir, "docker-compose.yml") + if err := os.WriteFile(bad, []byte(":\n - not: [valid"), 0o644); err != nil { + t.Fatal(err) + } + + if _, err := ComposeServiceNames([]string{bad}); err == nil { + t.Error("expected an error for a file that exists but fails to parse as YAML") + } +} diff --git a/internal/doctor/deep_docker.go b/internal/doctor/deep_docker.go index fbfc6f38..10509a16 100644 --- a/internal/doctor/deep_docker.go +++ b/internal/doctor/deep_docker.go @@ -57,8 +57,13 @@ func DockerDeepChecks(ctx context.Context, verbose bool) []CheckResult { status = parts[1] } if strings.Contains(strings.ToLower(status), "unhealthy") { - results = append(results, CheckResult{Section: "docker", Name: fmt.Sprintf("Container: %s", cName), - Status: "fail", Message: "unhealthy", FixCmd: fmt.Sprintf("docker restart %s", cName)}) + generic := CheckResult{Section: "docker", Name: fmt.Sprintf("Container: %s", cName), + Status: "fail", Message: "unhealthy", FixCmd: fmt.Sprintf("docker restart %s", cName)} + // G-014: an "unhealthy" status is only actionable if the + // healthcheck command itself can run inside the image — + // otherwise "docker restart" is a guess that never helps + // (see deep_docker_healthcheck.go). + results = append(results, diagnoseUnhealthyContainer(ctx, cName, generic)) } } } diff --git a/internal/doctor/deep_docker_healthcheck.go b/internal/doctor/deep_docker_healthcheck.go new file mode 100644 index 00000000..66af1b31 --- /dev/null +++ b/internal/doctor/deep_docker_healthcheck.go @@ -0,0 +1,163 @@ +package doctor + +// deep_docker_healthcheck.go — G-014 (second defect): a container whose +// healthcheck command cannot even execute inside its own image reports +// "unhealthy" forever, regardless of whether the service actually works. +// That status then carries zero information — worse than no healthcheck at +// all, because it looks like a real, actionable signal. +// +// WHY this lives in doctor, not build: `nself build` is a pure generator — +// it never touches the Docker daemon (see internal/build; nothing in that +// package imports internal/docker). Detecting a command that is not on the +// image's PATH requires a live container to exec into, which only exists +// once something is running. `nself doctor --deep` already inspects live +// container health in DockerDeepChecks (deep_docker.go) and already +// suggests `docker restart` for every "unhealthy" container — exactly the +// place the false signal was measured live: curl was never installed in +// nself-mux/nself-cron's image, so their healthcheck always errored, and +// "restart" was always the wrong suggestion (they were fine; a DB DNS +// failure elsewhere was the real, unrelated problem for two of them). +// +// Purpose: given a container already known to report "unhealthy", decide +// whether that status is trustworthy or the healthcheck command itself is +// unusable, and produce the right CheckResult either way. +// Inputs: ctx, the container name (from DockerDeepChecks' `docker ps` scan). +// Outputs: a CheckResult — the existing generic "unhealthy"/"docker restart" +// message when the healthcheck is fine or undeterminable, or a +// distinct "invalid healthcheck" message/fix when the command is +// confirmed missing from the image. +// Constraints: read-only (docker inspect + docker exec `command -v`, never +// a mutating command); best-effort — any ambiguity falls back +// to the pre-existing generic message rather than guessing. + +import ( + "context" + "encoding/json" + "fmt" + "os/exec" + "strings" +) + +// healthcheckConfig mirrors the subset of `docker inspect --format +// '{{json .Config.Healthcheck}}'` this check needs. +type healthcheckConfig struct { + Test []string `json:"Test"` +} + +// inspectHealthcheckTest returns the container's healthcheck Test slice — +// e.g. ["CMD-SHELL", "curl -f http://localhost:8080/health"] — or (nil, nil) +// when the image defines no healthcheck at all ("Healthcheck": null). +func inspectHealthcheckTest(ctx context.Context, containerName string) ([]string, error) { + cmd := exec.CommandContext(ctx, "docker", "inspect", "--format", "{{json .Config.Healthcheck}}", containerName) + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("docker inspect healthcheck for %s: %w", containerName, err) + } + trimmed := strings.TrimSpace(string(out)) + if trimmed == "" || trimmed == "null" { + return nil, nil + } + var hc healthcheckConfig + if err := json.Unmarshal([]byte(trimmed), &hc); err != nil { + return nil, fmt.Errorf("parsing healthcheck config for %s: %w", containerName, err) + } + return hc.Test, nil +} + +// extractHealthcheckBinary parses a Docker healthcheck Test slice and +// returns its mode ("CMD-SHELL", "CMD", "NONE", or "" if unrecognized) and +// the first binary the test would need to invoke. Pure — no docker access — +// so it is exercised directly in tests against canned Test slices. +func extractHealthcheckBinary(test []string) (mode, binary string) { + if len(test) == 0 { + return "", "" + } + mode = test[0] + switch mode { + case "NONE": + return mode, "" + case "CMD-SHELL": + if len(test) < 2 { + return mode, "" + } + fields := strings.Fields(test[1]) + if len(fields) == 0 { + return mode, "" + } + return mode, fields[0] + case "CMD": + if len(test) < 2 { + return mode, "" + } + return mode, test[1] + default: + // Older/uncommon shape without a mode marker — best-effort: the + // first element itself is the binary. + return "", test[0] + } +} + +// classifyBinaryProbe interprets the result of a `command -v ` +// probe run inside a container. ranToCompletion means the shell actually +// executed (exit 0 or a normal non-zero exit) rather than failing to start +// at all (no shell in the image, container not running, daemon +// unreachable) — only then is "not found" a conclusive signal. Pure — +// tested directly with canned stdout/ranToCompletion pairs. +func classifyBinaryProbe(stdout string, ranToCompletion bool) (found, verifiable bool) { + if !ranToCompletion { + return false, false + } + return strings.TrimSpace(stdout) != "", true +} + +// probeBinaryInContainer runs `docker exec sh -c "command -v +// "` and reports whether the binary was found, whether the probe +// was conclusive at all, and a hard error only when docker itself could not +// run the exec (container not running, daemon unreachable, etc.). +func probeBinaryInContainer(ctx context.Context, containerName, binary string) (found, verifiable bool, err error) { + cmd := exec.CommandContext(ctx, "docker", "exec", containerName, "sh", "-c", "command -v "+binary) + out, runErr := cmd.Output() + if runErr == nil { + found, verifiable = classifyBinaryProbe(string(out), true) + return found, verifiable, nil + } + if _, ok := runErr.(*exec.ExitError); ok { + // The shell ran and command -v exited non-zero: conclusively absent. + found, verifiable = classifyBinaryProbe(string(out), true) + return found, verifiable, nil + } + return false, false, fmt.Errorf("docker exec %s: %w", containerName, runErr) +} + +// diagnoseUnhealthyContainer decides whether an already-"unhealthy" +// container's status is trustworthy. genericResult is what the caller would +// otherwise report (the pre-existing "unhealthy" / "docker restart" +// message) — returned unchanged whenever the healthcheck can't be +// conclusively shown broken, so this never downgrades a real failure. +func diagnoseUnhealthyContainer(ctx context.Context, cName string, genericResult CheckResult) CheckResult { + test, err := inspectHealthcheckTest(ctx, cName) + if err != nil || len(test) == 0 { + return genericResult + } + mode, binary := extractHealthcheckBinary(test) + if mode == "NONE" || binary == "" { + return genericResult + } + + found, verifiable, probeErr := probeBinaryInContainer(ctx, cName, binary) + if probeErr != nil || !verifiable || found { + return genericResult + } + + return CheckResult{ + Section: "docker", + Name: fmt.Sprintf("Container: %s (invalid healthcheck)", cName), + Status: "fail", + Message: fmt.Sprintf( + "healthcheck command %q is not installed in this image — status is permanently \"unhealthy\" regardless of whether the service works, so it carries no information", + binary), + FixCmd: fmt.Sprintf( + "Install %q in the image, or change the healthcheck test to a command the image has — then: nself build --force && nself start", + binary), + } +} diff --git a/internal/doctor/deep_docker_healthcheck_test.go b/internal/doctor/deep_docker_healthcheck_test.go new file mode 100644 index 00000000..1fa4ae09 --- /dev/null +++ b/internal/doctor/deep_docker_healthcheck_test.go @@ -0,0 +1,117 @@ +package doctor + +// Tests for G-014's healthcheck-validity check. extractHealthcheckBinary and +// classifyBinaryProbe are pure — exercised directly, no live docker daemon +// or real project containers required. diagnoseUnhealthyContainer itself +// shells out (docker inspect/exec) so it is not unit-tested here; its +// building blocks are, which is what actually encodes the decision logic. + +import "testing" + +func TestExtractHealthcheckBinary(t *testing.T) { + cases := []struct { + name string + test []string + wantMode string + wantBinary string + }{ + { + name: "CMD-SHELL curl (the measured prod case)", + test: []string{"CMD-SHELL", "curl -f http://localhost:8080/health || exit 1"}, + wantMode: "CMD-SHELL", + wantBinary: "curl", + }, + { + name: "CMD exec-form", + test: []string{"CMD", "curl", "-f", "http://localhost:8080/health"}, + wantMode: "CMD", + wantBinary: "curl", + }, + { + name: "CMD-SHELL wget", + test: []string{"CMD-SHELL", "wget --spider -q http://localhost/health"}, + wantMode: "CMD-SHELL", + wantBinary: "wget", + }, + { + name: "NONE disables the healthcheck", + test: []string{"NONE"}, + wantMode: "NONE", + wantBinary: "", + }, + { + name: "empty test slice", + test: nil, + wantMode: "", + wantBinary: "", + }, + { + name: "CMD-SHELL with no command string", + test: []string{"CMD-SHELL"}, + wantMode: "CMD-SHELL", + wantBinary: "", + }, + { + name: "CMD-SHELL with blank command string", + test: []string{"CMD-SHELL", " "}, + wantMode: "CMD-SHELL", + wantBinary: "", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + mode, binary := extractHealthcheckBinary(c.test) + if mode != c.wantMode { + t.Errorf("mode = %q, want %q", mode, c.wantMode) + } + if binary != c.wantBinary { + t.Errorf("binary = %q, want %q", binary, c.wantBinary) + } + }) + } +} + +func TestClassifyBinaryProbe(t *testing.T) { + cases := []struct { + name string + stdout string + ranToCompletion bool + wantFound bool + wantVerifiable bool + }{ + { + name: "binary found: command -v prints its path", + stdout: "/usr/bin/curl\n", + ranToCompletion: true, + wantFound: true, + wantVerifiable: true, + }, + { + name: "binary confirmed missing: exit ran, empty stdout", + stdout: "", + ranToCompletion: true, + wantFound: false, + wantVerifiable: true, + }, + { + name: "probe never ran (no shell in image, daemon unreachable, etc.)", + stdout: "", + ranToCompletion: false, + wantFound: false, + wantVerifiable: false, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + found, verifiable := classifyBinaryProbe(c.stdout, c.ranToCompletion) + if found != c.wantFound { + t.Errorf("found = %v, want %v", found, c.wantFound) + } + if verifiable != c.wantVerifiable { + t.Errorf("verifiable = %v, want %v", verifiable, c.wantVerifiable) + } + }) + } +}