From a90496bcbc82217ecbdd0736524c35e0f33c8787 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Wed, 28 Oct 2026 17:57:26 -0400 Subject: [PATCH 1/4] fix(generate): reap the agents `generate contracts` starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `generate contracts` reads each service's endpoints through generators.LoadServiceEndpoints, which calls instance.LoadBuilder and so spawns an agent subprocess per service. The command never closed the cached connections, so every pass left those agents running: two regenerations of one workspace accumulated eight live private go-grpc processes, with nothing failing and the command exiting 0. Closing the connections is what ends them — the agent's own Serve installs a SIGTERM handler and shuts down inside the parent's window — and every sibling that loads agents already does it, `generate client` included. This adds the same `defer services.ClearAgents()`. The guard is per COMMAND FILE because the two obvious granularities are both wrong, and the looser one let this very bug through while I was writing it: a file that declares a command and reaches a spawn must reap in that same file. Per-file alone would wrongly flag cmd/sourcefix/fix.go, which loads while its sibling commands.go defers the reap. Per-package would clear cmd/generate/contracts.go, because client.go's reap sits in the same package — checked, and it did. A helper file with no RunE is cleared, since whichever RunE calls it owns the lifetime. Already-running agents are not retired by this; they need an explicit `recover process --expect FILE --graceful` each. Co-Authored-By: Claude Opus 5 --- cmd/generate/contracts.go | 10 ++++ pkg/conformance/agent_reaping_test.go | 82 +++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 pkg/conformance/agent_reaping_test.go diff --git a/cmd/generate/contracts.go b/cmd/generate/contracts.go index 66924da7..1a7c7167 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,15 @@ 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 -> instance.LoadBuilder), which + // spawns an agent subprocess per service. Closing the cached + // connections is what ends them: the agent's own Serve installs a + // SIGTERM handler and shuts down inside the parent's window. 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. + defer services.ClearAgents() if contractsFormat != "text" && contractsFormat != "json" { return fmt.Errorf("--format must be text or json, got %q", contractsFormat) diff --git a/pkg/conformance/agent_reaping_test.go b/pkg/conformance/agent_reaping_test.go new file mode 100644 index 00000000..65267a40 --- /dev/null +++ b/pkg/conformance/agent_reaping_test.go @@ -0,0 +1,82 @@ +package conformance + +import ( + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// TestEveryCommandThatLoadsAgentsReapsThem stops a command from spawning agent +// subprocesses and leaving them running. +// +// Loading a builder or runtime starts an agent process per service. Closing the +// cached connections is what ends them — the agent's own Serve installs a +// SIGTERM handler and shuts down inside the parent's window — so a command that +// loads agents and never calls services.ClearAgents leaks one process per +// service, every invocation. +// +// 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 TestEveryCommandThatLoadsAgentsReapsThem(t *testing.T) { + root := repositoryRoot(t) + // Calls that start an agent subprocess for a service. + spawns := []string{"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 := 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) + } +} From eb42324f36e5fad688800597ad4106be536b49b2 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Wed, 28 Oct 2026 18:15:13 -0400 Subject: [PATCH 2/4] docs(generate): say what the reap does, and narrow the guard's claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three corrections from review, all of them things the first version overstated or got wrong. My comment said closing the connections works because the agent's Serve installs a SIGTERM handler. Core's own doc is more exact and contradicts the emphasis: conn.Close() is graceful in itself — SIGTERM, wait, SIGKILL fallback — and an explicit pre-signal was removed from ClearAgents as "redundant and harmful" because it raced the agent's handler. The comment now says that, and says plainly that this retires the processes the command started and claims nothing about agents another command owns. The spawn list gains services.Load, which is where the agent process is actually started; LoadBuilder and LoadRuntime then attach to it. Adding it flags nothing new today, which is the point — it closes the hole where a command spawns without naming either attach call. The guard is renamed and described as what it is: a TEXTUAL omission detector. It reads source as text, so a `// TODO: ClearAgents` comment defeats it. It catches the omission that happened and guards against the same slip recurring; it is not proof that any command reaps correctly, and deliberately not a call-graph analysis. docs/commands.md now documents the ordinary return and the cooperative cancel (the reap is deferred, so Ctrl-C takes the same path), distinguishes that from a forced exit, and points an already-orphaned agent at `recover process --graceful` by identity rather than implying this command adopts it. Co-Authored-By: Claude Opus 5 --- cmd/generate/contracts.go | 18 ++++++++++------- docs/commands.md | 16 +++++++++++++++ pkg/conformance/agent_reaping_test.go | 28 ++++++++++++++++++--------- 3 files changed, 46 insertions(+), 16 deletions(-) diff --git a/cmd/generate/contracts.go b/cmd/generate/contracts.go index 1a7c7167..c4c80439 100644 --- a/cmd/generate/contracts.go +++ b/cmd/generate/contracts.go @@ -78,13 +78,17 @@ against what's on disk, without writing anything. ctx, stop := common.SignalContext(ctx) defer stop() // This command loads service builders to read their endpoints - // (generators.LoadServiceEndpoints -> instance.LoadBuilder), which - // spawns an agent subprocess per service. Closing the cached - // connections is what ends them: the agent's own Serve installs a - // SIGTERM handler and shuts down inside the parent's window. 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. + // (generators.LoadServiceEndpoints -> services.Load, which spawns the + // agent, then instance.LoadBuilder), so an agent subprocess exists per + // service. ClearAgents closes the cached connections, and the close is + // itself graceful in Core (services/agent.go: SIGTERM, wait, SIGKILL + // fallback) so the agent reaps its own children first. 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" { diff --git a/docs/commands.md b/docs/commands.md index 554ae783..82d3629d 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3034,6 +3034,22 @@ 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: the cached connections are +closed, and the close is cooperative — Core signals the agent, waits for it to +reap its own children (user binaries, containers) and only then forces exit, so +a shutdown that completes is a clean one. Cancelling the command (Ctrl-C) takes +the same path, because the reap is deferred rather than conditional on success. + +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, and `codefly recover process --preview` +then `--expect FILE --graceful` is how one is retired explicitly, by identity +and one at a time, with no group or registry effect. + + 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 index 65267a40..6b738265 100644 --- a/pkg/conformance/agent_reaping_test.go +++ b/pkg/conformance/agent_reaping_test.go @@ -8,14 +8,21 @@ import ( "testing" ) -// TestEveryCommandThatLoadsAgentsReapsThem stops a command from spawning agent -// subprocesses and leaving them running. +// TestNoCommandFileOmitsAgentReaping is a TEXTUAL omission detector, and +// nothing more: it reads source as text 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 — and it is defeated by +// anything that merely mentions the call, a `// TODO: ClearAgents` comment +// included. 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. // -// Loading a builder or runtime starts an agent process per service. Closing the -// cached connections is what ends them — the agent's own Serve installs a -// SIGTERM handler and shuts down inside the parent's window — so a command that -// loads agents and never calls services.ClearAgents leaks one process per -// service, every invocation. +// Loading a service starts an agent process. ClearAgents closes the cached +// connections and Core's close is graceful in itself (services/agent.go: +// SIGTERM, wait, SIGKILL fallback), so 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 @@ -26,10 +33,13 @@ import ( // 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 TestEveryCommandThatLoadsAgentsReapsThem(t *testing.T) { +func TestNoCommandFileOmitsAgentReaping(t *testing.T) { root := repositoryRoot(t) // Calls that start an agent subprocess for a service. - spawns := []string{"LoadBuilder(", "LoadRuntime(", "LoadServiceEndpoints("} + // 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"} From 98419247c4411bc510adb56e29ea8aaf9bcfa19d Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:32:49 -0400 Subject: [PATCH 3/4] fix(conformance): make the reap guard see code, not comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard matched the word "ClearAgents" anywhere in a command file, so the explanatory comment above the real `defer services.ClearAgents()` satisfied it on its own: deleting the defer and keeping the comment passed. It now strips comments by parsing each file and printing the tree back before matching, so a reap has to be a reap. Verified against that exact mutation — defer removed, comment left in place — which the guard now fails on. Two factual corrections alongside it: - `docs/commands.md` pointed at `recover process --graceful`, a flag that does not exist at this version (it is proposed in a separate change). It now names `--preview` plus `--expect` with an explicit mode, says the only mode available here is `--force`, and says plainly that an irreversible SIGKILL is not a substitute for a cooperative shutdown — it gives the agent no chance to reap its children. What should happen when a target refuses to exit, or is signalled twice, is called out as not covered. - The comment on the reap described Core's close as "SIGTERM, wait, SIGKILL fallback". Core sends os.Interrupt; its own comments there disagree with its code. Both this comment and the guard's now describe the owner seam — ask, wait for the agent to reap its children, force the group down only on timeout — without naming a signal that is Core's to choose. Co-Authored-By: Claude Opus 5 --- cmd/generate/contracts.go | 9 +++-- docs/commands.md | 25 +++++++++----- pkg/conformance/agent_reaping_test.go | 47 +++++++++++++++++++++------ 3 files changed, 60 insertions(+), 21 deletions(-) diff --git a/cmd/generate/contracts.go b/cmd/generate/contracts.go index c4c80439..7dfb0aae 100644 --- a/cmd/generate/contracts.go +++ b/cmd/generate/contracts.go @@ -80,9 +80,12 @@ against what's on disk, without writing anything. // 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 the close is - // itself graceful in Core (services/agent.go: SIGTERM, wait, SIGKILL - // fallback) so the agent reaps its own children first. Without this, + // 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. diff --git a/docs/commands.md b/docs/commands.md index 82d3629d..d03902eb 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3036,18 +3036,27 @@ release commit also: **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: the cached connections are -closed, and the close is cooperative — Core signals the agent, waits for it to -reap its own children (user binaries, containers) and only then forces exit, so -a shutdown that completes is a clean one. Cancelling the command (Ctrl-C) takes -the same path, because the reap is deferred rather than conditional on success. +process. The command retires them on return by closing the cached connections. +That close is Core's, and it is cooperative: Core asks the agent to stop, waits +for the agent to reap its own children (user binaries, containers), and forces +the process group down only if that wait times out. The signal it uses is +Core's own choice and is not restated here. Cancelling the command (Ctrl-C) +takes the same path, because the reap is deferred rather than conditional on +success. 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, and `codefly recover process --preview` -then `--expect FILE --graceful` is how one is retired explicitly, by identity -and one at a time, with no group or registry effect. +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 diff --git a/pkg/conformance/agent_reaping_test.go b/pkg/conformance/agent_reaping_test.go index 6b738265..f59026fa 100644 --- a/pkg/conformance/agent_reaping_test.go +++ b/pkg/conformance/agent_reaping_test.go @@ -1,6 +1,10 @@ package conformance import ( + "bytes" + "go/parser" + "go/printer" + "go/token" "os" "path/filepath" "sort" @@ -8,18 +12,41 @@ import ( "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 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 — and it is defeated by -// anything that merely mentions the call, a `// TODO: ClearAgents` comment -// included. 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. +// 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 graceful in itself (services/agent.go: -// SIGTERM, wait, SIGKILL fallback), so a command that loads agents and never +// 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. @@ -59,7 +86,7 @@ func TestNoCommandFileOmitsAgentReaping(t *testing.T) { if err != nil || entry.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { return err } - body := readFile(t, path) + body := codeOf(t, path, readFile(t, path)) if !strings.Contains(body, "RunE:") { return nil } From b975d729ead2c3f381c7611d3303271b85aa78f3 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:33:23 -0400 Subject: [PATCH 4/4] docs(generate): state what the reap attempts, not what it guarantees The page said Core "waits for it to reap its own children ... so a shutdown that completes is a clean one". The pinned implementation does not establish that: the close reports no error, the agent ignores the error from stopping its children, and the agent has its own hard-deadline exit. An agent process that is gone proves the agent exited, not that nothing it started survived. The text now describes best-effort cooperative cleanup with a forced fallback and says plainly what is not certified. It also conflated the two cancellation paths. One Ctrl-C runs the deferred reap; a second one exits immediately and runs no defers, which leaves behind exactly the agents this change exists to prevent. That is now stated. Co-Authored-By: Claude Opus 5 --- docs/commands.md | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index d03902eb..959e93de 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3037,12 +3037,21 @@ release commit also: **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 it is cooperative: Core asks the agent to stop, waits -for the agent to reap its own children (user binaries, containers), and forces -the process group down only if that wait times out. The signal it uses is -Core's own choice and is not restated here. Cancelling the command (Ctrl-C) -takes the same path, because the reap is deferred rather than conditional on -success. +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