diff --git a/cmd/generate/contracts.go b/cmd/generate/contracts.go index 66924da7..7dfb0aae 100644 --- a/cmd/generate/contracts.go +++ b/cmd/generate/contracts.go @@ -21,6 +21,7 @@ import ( basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" resources "github.com/codefly-dev/core/resources" runners "github.com/codefly-dev/core/runners/dockerrun" + "github.com/codefly-dev/core/services" "github.com/codefly-dev/core/shared" "github.com/codefly-dev/core/standards" "github.com/spf13/cobra" @@ -76,6 +77,22 @@ against what's on disk, without writing anything. defer done() ctx, stop := common.SignalContext(ctx) defer stop() + // This command loads service builders to read their endpoints + // (generators.LoadServiceEndpoints -> services.Load, which spawns the + // agent, then instance.LoadBuilder), so an agent subprocess exists per + // service. ClearAgents closes the cached connections, and Core's close + // is cooperative on its own account: it asks the agent to stop, waits + // for the agent to reap its own children, and forces the process group + // down only if that wait expires. Which signal it sends is Core's to + // decide and is not restated here — the comments around it in Core name + // SIGTERM while the code sends os.Interrupt. Without this, + // every `generate contracts` pass left its agents running — eight live + // private go-grpc records accumulated across two regenerations of one + // workspace. `generate client` already reaps the same way. + // + // It ends the processes THIS command started, which is all it claims: + // an agent already running from another command is not its business. + defer services.ClearAgents() if contractsFormat != "text" && contractsFormat != "json" { return fmt.Errorf("--format must be text or json, got %q", contractsFormat) diff --git a/docs/commands.md b/docs/commands.md index 554ae783..959e93de 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3034,6 +3034,40 @@ release commit also: in full. A catalog that no longer matches its contract files is refused — run `codefly generate contracts` and commit it first. +**Agents the command starts, and what ends them.** Reading each service's +exported endpoints loads that service, which starts its agent as a child +process. The command retires them on return by closing the cached connections. +That close is Core's, and what it attempts is cooperative: Core asks the agent +to stop, gives it time to retire its own children (user binaries, containers), +and forces the process group down if that wait expires. The signal it sends is +Core's own choice and is not restated here. + +What that does **not** establish is that every child was retired. The close +reports no error, the agent ignores the error from stopping its children, and +the agent has a hard-deadline exit of its own, so an agent process that has +gone away is evidence the agent exited — not proof that nothing it started +survived it. Treat this as best-effort cleanup with a forced fallback. + +Cancelling the command once (Ctrl-C) takes the same path: the reap is deferred +rather than conditional on success, so it still runs. A **second** Ctrl-C does +not — it exits the process immediately and no deferred cleanup runs at all, +which leaves exactly the agents this retirement exists to prevent. + +It retires the agents **that run of the command started**. An agent left +running by an earlier command — a version of the CLI before this retirement +existed, for instance — is not adopted or reaped here: it is a separate process +that outlived its own command. Retiring one is `codefly recover process +--preview` to read its identity, then `--expect FILE` with an explicit mode to +act on exactly that birth, one process at a time, with no group or registry +effect. + +The only mode this version offers is `--force`, an irreversible SIGKILL. That +is **not** a substitute for a cooperative shutdown: it gives the agent no +chance to reap its own children, so a leftover agent's user binaries and +containers can survive it. A graceful mode, and what should happen when a +target refuses to exit or is signalled twice, are not covered at this version. + + The services and modules are found through `workspace.codefly.yaml` and each `module.codefly.yaml` — never by searching the tree, so a fixture workspace elsewhere in the repository is not touched, and never through the machine-local diff --git a/pkg/conformance/agent_reaping_test.go b/pkg/conformance/agent_reaping_test.go new file mode 100644 index 00000000..f59026fa --- /dev/null +++ b/pkg/conformance/agent_reaping_test.go @@ -0,0 +1,119 @@ +package conformance + +import ( + "bytes" + "go/parser" + "go/printer" + "go/token" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// codeOf returns the file's source with every comment removed, by parsing it +// and printing the syntax tree back out. The checks below are textual, and a +// comment is text: the first version of this guard matched the word +// "ClearAgents" anywhere in the file, so the explanatory comment that sits +// above the real `defer services.ClearAgents()` was itself enough to satisfy +// it — deleting the defer and keeping the comment passed. Stripping comments +// first is what makes a reap have to be a reap. +func codeOf(t *testing.T, path, body string) string { + t.Helper() + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, body, 0) + if err != nil { + t.Fatalf("%s: %v", path, err) + } + var code bytes.Buffer + if err := printer.Fprint(&code, fset, file); err != nil { + t.Fatalf("%s: %v", path, err) + } + return code.String() +} + +// TestNoCommandFileOmitsAgentReaping is a TEXTUAL omission detector, and +// nothing more: it reads source as text — comments stripped — and asks whether +// a reaping call appears. It catches the omission that actually happened: a +// command that loads agents with no reap anywhere in its file. It is a cheap +// backstop against the same slip recurring, not proof that any command reaps +// correctly, and it is deliberately not a call-graph analysis: a reap written +// behind a condition that never runs would still satisfy it. +// +// Loading a service starts an agent process. ClearAgents closes the cached +// connections, and Core's close is cooperative in itself — it asks the agent +// to stop, waits for the agent to reap its own children, and forces the +// process group down only if that wait times out. (The signal is Core's +// choice; the comments around it in Core do not match the code it runs, so +// this does not restate a mechanism.) A command that loads agents and never +// reaps leaks one process per service, every invocation. It retires the +// processes that command started; agents owned by another command are not +// affected. +// +// This is not hypothetical: `generate contracts` loaded builders through +// generators.LoadServiceEndpoints and reaped nothing, while its sibling +// `generate client` reaped correctly. Two regenerations of one workspace left +// eight live private go-grpc agents behind. The omission was invisible because +// nothing failed — the command exited 0 and the processes simply stayed. +// +// A command that deliberately hands its agents to something longer-lived +// (a server, the MCP host) still has to say so by name below, so the exemption +// is a decision on the record rather than an absence. +func TestNoCommandFileOmitsAgentReaping(t *testing.T) { + root := repositoryRoot(t) + // Calls that start an agent subprocess for a service. + // services.Load is where the agent process is actually started; + // LoadBuilder/LoadRuntime then attach to it. All of them are listed because + // a command reaching any of them has an agent to answer for. + spawns := []string{"services.Load(", "LoadBuilder(", "LoadRuntime(", "LoadServiceEndpoints("} + // Reaping, however the command spells it. + reaps := []string{"ClearAgents", "ClearAgent(", "RegisterCleanup(services.ClearAgents"} + + // Per COMMAND FILE, which is the granularity that actually holds. + // + // Per-file alone is too strict: cmd/sourcefix/fix.go loads while its + // sibling commands.go defers the reap, and that is correct. Per-package is + // too loose: cmd/generate hosts many independent commands, so + // client.go's reap masked contracts.go having none — the very bug this + // guard exists for passed a per-package check. + // + // So the invariant is: a file that declares a command (RunE) AND reaches a + // spawn must reap in that same file. A helper file with no RunE is cleared, + // because whichever RunE calls it owns the lifetime. + var offenders []string + err := filepath.WalkDir(filepath.Join(root, "cmd"), func(path string, entry os.DirEntry, err error) error { + if err != nil || entry.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return err + } + body := codeOf(t, path, readFile(t, path)) + if !strings.Contains(body, "RunE:") { + return nil + } + loads := false + for _, call := range spawns { + loads = loads || strings.Contains(body, call) + } + if !loads { + return nil + } + for _, reap := range reaps { + if strings.Contains(body, reap) { + return nil + } + } + relative, relErr := filepath.Rel(root, path) + if relErr != nil { + relative = path + } + offenders = append(offenders, filepath.ToSlash(relative)) + return nil + }) + if err != nil { + t.Fatal(err) + } + sort.Strings(offenders) + for _, offender := range offenders { + t.Errorf("%s declares a command that loads service agents but never reaps them: add `defer services.ClearAgents()` beside the command's other defers, or name the file here with the reason its agents outlive it", offender) + } +}