Repository navigation
fix(generate): reap the agents generate contracts starts - #1224
Merged
Merged
Conversation
`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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
codefly generate contractsspawned an agent subprocess per service and never ended them. SaaS accumulated eight live privatego-grpcagents across two regenerations of one workspace — four, then four more — with nothing failing and the command exiting 0. Classification: fix at the CLI-owned agent lifecycle boundary. Related: #932.Cause
cmd/generate/contracts.go:234→generators.LoadServiceEndpoints→instance.LoadBuilder(ctx)(pkg/generators/contracts.go:20), which starts the service's agent to askBuilder.Loadfor its endpoints. The command'sRunEthen returned with the cached connections never closed.Closing them is what ends the agents: the agent's own
Serveinstalls a SIGTERM handler and shuts down inside the parent's window (core/agents/agents.go:469-504). Every sibling that loads agents already reaps —cmd/test/service.go:62,cmd/test/source.go:55,cmd/sbom/sbom.go:34,69,cmd/upgrade/{workspace,service}.go,cmd/sourcefix/commands.go:45,69,cmd/server.go:36,cmd/mcp.go:60,93,cmd/delete/service.go:28— and withingenerate,client.goreaps at lines 70 and 201 whilecontracts.goreaped nowhere. So this was an omission in one command, not a design choice. One line restores it.The guard, and why its granularity is what it is
TestEveryCommandThatLoadsAgentsReapsThemchecks per command file: a file that declares a command (RunE) and reaches a spawn must reap in that same file. Both obvious alternatives are wrong, and I only know that because the looser one let this bug through while I was writing the guard:cmd/generatehosts many independent commands, soclient.go's reap sits in the same package ascontracts.go. I wrote the per-package version first, reverted the fix to check it, and it passed — the guard would have shipped unable to catch the thing it was written for.cmd/sourcefix/fix.goloads agents while its siblingcommands.godefers the reap, which is correct; a per-file rule flagged it.A helper file with no
RunEis cleared, because whicheverRunEcalls it owns the lifetime. An exemption has to be written in by name, so "these agents outlive the command" is a decision on the record rather than an absence.Verified both directions: the guard passes with the fix, and reverting the
defermakes it fail namingcmd/generate/contracts.go.Checks
go build ./...clean,go vet ./...clean,golangci-lint run ./cmd/generate/... ./pkg/conformance/...0 issues,./cmd/generatepasses (5.6s), the new guard passes. The fourpkg/conformancefailures seen locally (TestMarkerProjectionSitesMatchTheInventory,TestContainerRecoveryScopeHasOneResolver,TestGenerateQualificationScriptRunsTheMatrixRow,TestDeclaredGatesMatchCallSites) are pre-existing and environment-only — I reproduced the identical four at cleanorigin/mainin a separate checkout; they scan the repo tree and trip over an untracked.lazybox/checkouts/.Limits
This stops new orphans. It does not retire the eight already running — those need an explicit
codefly recover process <PID> --previewthen--expect FILE --gracefuleach, which is #1222. No broad cleanup, no sweep, no foreign registry effect.🤖 Generated with Claude Code