ate-env-guest: stop a sidecar's whole process group on shutdown by tomergee · Pull Request #79 · agent-substrate/env

Tomer Glottmann (tomergee) · 2026-10-13T18:35:20Z

Why

runSidecar used exec.CommandContext, whose cancel kills only the direct child. A sidecar that forks, for example a wrapper script, which docs/rootfs-images/RUNTIMES.md recommends for quoted arguments, died alone while its child kept the output pipes open, and cmd.Wait blocked until that child exited. The guest's shutdown hung for as long as the orphan lived.

TestRunSidecarRestartsAndStopsWithContext caught this on Linux and not on macOS: sh -c "sleep 60" is dash on Linux, which forks sleep, and bash on macOS, which execs it. First seen in the ci run on #71, the first Linux run of this code; main has no CI yet.

What

Each sidecar runs in its own process group (Setpgid). On cancel the group gets SIGTERM, cmd.WaitDelay (sidecarStopGrace, 5 s) bounds the wait for the pipes, and the group gets SIGKILL for anything still alive. signalGroup treats ESRCH as success. Same shape as the guest's own process tracker.

Testing

golang:1.27 container: the test 3/3 before the fix fails 3/3, after the fix passes 3/3; ./cmd/ate-env-guest/ and ./guest/... green.
macOS: the test and the full suite pass.
Second commit (922a95b, from review): main waits for the supervisors after Serve returns, the old group is SIGKILLed before a restart too, a stop is logged, and the test forks on every platform (sleep 60 & wait) so macOS exercises the group kill as well.
Third commit (ec682ef) is Publish release images to GHCR and a CLI that defaults to them #71's gofmt internal/ate carried here, so this tree passes the gofmt gate Publish release images to GHCR and a CLI that defaults to them #71 adds whichever PR lands first. The later rebase drops the duplicate.
Full CI-equivalent run on Linux (golang:1.27, python:3.12 containers) against this tree: build, vet, gofmt, go test ./... (12 packages), go test -race over cmd, guest and internal, license headers, and the Python client's 73 tests all pass.
Publish release images to GHCR and a CLI that defaults to them #71 carries the two sidecar commits as cherry-picks so its CI is green; the rebase after this merges drops the duplicates.

runSidecar used exec.CommandContext, whose cancel kills only the direct child. A sidecar that forks (a wrapper script, which RUNTIMES.md recommends for quoted arguments) died alone while its child kept the output pipes open, and cmd.Wait blocked until that child exited. The guest's shutdown hung for as long as the orphan lived. TestRunSidecarRestartsAndStopsWithContext caught this on Linux and not on macOS: `sh -c "sleep 60"` is dash on Linux, which forks sleep, and bash on macOS, which execs it. First seen in the ci run on #71 (https://github.com/agent-substrate/env/actions/runs/38440900526), the first Linux run of this code. Each sidecar now runs in its own process group. On cancel the group gets SIGTERM, cmd.WaitDelay (sidecarStopGrace, 5 s) bounds the wait for the pipes, and the group gets SIGKILL for anything still alive. Verified in a golang:1.27 container, the test 3/3 and the guest packages green, and on macOS.

… platform Review follow-ups on the process-group fix: - main waits for the supervisors after Serve returns (bounded by sidecarStopGrace plus a second), so the SIGTERM, grace and SIGKILL sequence runs before the process exits. Inside an actor the guest is PID 1 and the container ends with it anyway; outside one, this is what stops the sidecars. - The old process group is SIGKILLed before a restart as well, so a restarted sidecar does not compete with its own orphans for ports. - A stop is logged. - The test forks explicitly (`sleep 60 & wait`): a bare `sh -c "sleep 60"` execs on macOS and did not exercise the group kill there. The bound is derived from sidecarStopGrace and explained: the forked sleep holds the pipe, so returning within it proves the child died.

Tomer Glottmann (tomergee) · 2026-10-13T18:47:09Z

Review of the first commit (93c5fb2), with the outcome. Blocking: none.

Before merge, all applied in 922a95b:

cmd/ate-env-guest/main.go:117 — go runSidecar(...) was never waited for. After srv.Serve returned, main returned and the process exited while runSidecar sat between SIGTERM and SIGKILL. Inside an actor the guest is PID 1, thus the container ends with it; outside one the sidecar stayed alive. Fixed: a sync.WaitGroup and a bounded wait (sidecarStopGrace + 1 s) after Serve returns.
cmd/ate-env-guest/sidecar.go, restart path — a sidecar that exited on its own and left children got restarted next to them, and competed with its own orphans for its port. Fixed: the old group gets SIGKILL before a restart as well.
cmd/ate-env-guest/sidecar_test.go:118 — sh -c "sleep 60" forks on Linux and execs on macOS, thus macOS never exercised the group kill. Fixed: sleep 60 & wait forks on every platform; the bound is derived from sidecarStopGrace and the comment explains why a return within it proves the child died.

Nits: a comment on the pid-reuse window after Wait (applied), and a log line on stop (applied; the test asserts it).

Verified on macOS and in a golang:1.27 container: the test 3/3 and ./cmd/ate-env-guest/ ./guest/... green on both.

…ting check (cherry picked from commit 86eebcc)

Tomer Glottmann (tomergee) · 2026-10-13T18:58:36Z

Full CI-equivalent run on Linux against ec682ef, in the images #71's CI uses (golang:1.27, python:3.12). GitHub does not run CI on this PR because the workflow lives in #71 and pull_request runs use the PR's own tree, thus this is the Linux evidence until #71 merges.

Step	Result
`go build ./...`	ok
`go vet ./...`	ok
gofmt over all Go files	ok (after `ec682ef`, #71's formatting commit carried here)
`go test ./...`	12 packages ok
`go test -race ./cmd/... ./guest/... ./internal/...`	ok
`hack/verify/boilerplate.sh`	ok
`clients/python` pytest	73 passed, e2e skipped (no `ATE_ENV_API_TARGET`)

TestRunSidecarRestartsAndStopsWithContext on Linux: fails 3/3 on main, passes 3/3 on this branch; the test now forks on every platform (sleep 60 & wait), thus macOS covers the same path.

The first pass of this run failed the gofmt gate on internal/ate/client.go and client_test.go, which #78 left unformatted on main. That is why ec682ef is here: the PR passes #71's gate whichever of the two lands first, and the later rebase drops the duplicate.

Sunny (sunnylovestiramisu) · 2026-10-13T20:01:44Z

Please centralize the shutdown/restart cleanup sequence so all sidecar stop paths use the same process-group SIGTERM → grace → SIGKILL flow, including the old-group cleanup before replacement. If the cleanup path is not fully consistent, future changes can reintroduce an orphan process or a shutdown hang.

If we do not make the cleanup path consistent What can go wrong:

Shutdown path and restart path diverge.
- One path kills the whole process group.
- The other path only kills the direct child or kills the group too late.
Restart races appear.
- The old sidecar’s child may still be alive when the new one starts.
- Ports, pipes, temp files, or lock files can still be held.
- The new sidecar may fail to bind or behave inconsistently.
Orphans survive the shutdown sequence.
- On a normal stop, the parent may exit before the whole group is actually dead.
- The old group lingers and can keep stdout/stderr or FDs open.
Flaky behavior across platforms or workloads.
- A wrapper script with a shell can behave differently than a straight binary.
- A forked child may survive the first kill path but not the second, depending on execution timing.

Please add explicit logs for the termination sequence: SIGTERM sent to the sidecar process group, wait for sidecarStopGrace, SIGKILL sent if still alive, and ESRCH treated as success. This will make shutdown behavior observable and easier to reason about under failure.

…p logs Review follow-up on #79. stopGroup is now the only way a sidecar's group is taken down: SIGTERM, up to sidecarStopGrace for the group to disappear, then SIGKILL for what is left. Both paths use it: after a cancel, where it finishes what cmd.Cancel started, and before a restart, where it clears what a crashed sidecar left behind so the new one does not compete with its own orphans for ports, pipes or lock files. Each step is logged: SIGTERM sent, group exited, still alive after the grace and SIGKILL sent, or already gone (ESRCH counts as success). main's shutdown wait covers both grace periods. The test asserts the logged sequence.

…p logs Review follow-up on #79. stopGroup is now the only way a sidecar's group is taken down: SIGTERM, up to sidecarStopGrace for the group to disappear, then SIGKILL for what is left. Both paths use it: after a cancel, where it finishes what cmd.Cancel started, and before a restart, where it clears what a crashed sidecar left behind so the new one does not compete with its own orphans for ports, pipes or lock files. Each step is logged: SIGTERM sent, group exited, still alive after the grace and SIGKILL sent, or already gone (ESRCH counts as success). main's shutdown wait covers both grace periods. The test asserts the logged sequence. (cherry picked from commit 8eb8274)

Tomer Glottmann (tomergee) · 2026-10-13T20:17:53Z

Both done in 8eb8274.

stopGroup is now the one way a sidecar's process group is taken down: SIGTERM, up to sidecarStopGrace for the group to disappear, then SIGKILL for what is left. The shutdown path calls it after Wait to finish what cmd.Cancel started; the restart path calls it before the next start to clear what a crashed sidecar left behind. There is no other kill path left in the file.
Each step is logged: context done, SIGTERM to process group N, SIGTERM to process group N, waiting up to 5s, process group N exited after SIGTERM, process group N still alive after 5s, SIGKILL, or process group N already exited for ESRCH. The test asserts the sequence.

main's shutdown wait now covers both grace periods. Verified on macOS and in a golang:1.27 container: the test 3/3 and the guest packages green on both. #71 carries the commit as a cherry-pick like the others.

* Publish release images and a CLI that knows them Nothing published images for this repo: the Makefile pushed to a personal registry, releases carried no assets, and the manifest command's error message pointed at "images published by the latest release" that did not exist. Every user, including the quickstart, had to build and push ate-env-api and ate-env-guest first. A release workflow now runs on every v* tag (and by hand for an existing tag): ko builds both images for linux/amd64 and linux/arm64, pushes them to ghcr.io/<org>/<repo>/ate-env-api and ate-env-guest tagged with the release and latest, signs them with cosign keyless signing bound to the workflow's identity, then builds the ate-env CLI for linux and darwin on amd64 and arm64 with the two digests and the version baked in, uploads the binaries, an images_<tag>.txt and checksums as release assets, and writes an Images section with the digests and the verify command into the release notes. The CLI gains build-time defaults: a release binary's `manifest --api-image` and `manifest template --guest-image` default to the images published with it, `ate-env --version` prints the tag, and a source build keeps the flags required with messages that say why. The worker image stays a required flag, since ateom-gvisor comes from the Substrate repo and must match the Substrate version on the cluster; the message says so. `make build-cli` reproduces a release-style binary locally. A ci workflow runs the Go build, vet and tests, the license-header check, and the Python client tests on pull requests and main; the repo had no checks before. The guest's bash base image is pinned by digest in .ko.yaml so a published guest is reproducible. docs/release.md describes the artifacts, pinning, verification, cutting and republishing a release, and the one-time organization setup for GitHub Container Registry; the README quickstart no longer needs image flags with a release binary. * workflows: pin actions by SHA, scope permissions, keep expressions out of shell What the organization's workflow security scan (zizmor) requires, and what a release pipeline should do anyway: - every action pinned by commit SHA, with the version in a trailing comment (each SHA checked against its tag) - checkout with persist-credentials off - top-level permissions empty; the release job asks for contents, packages and id-token itself - no expression interpolated into a run block: inputs and step outputs reach the shell through env - Go module cache off for the release build, so a release cannot consume a cache another run seeded - concurrency: a new push supersedes running CI; one publish per tag at a time docs/release.md explains the pinning convention and how to bump a pin. zizmor reports no findings on both workflows. * Makefile: images prints the digests instead of editing the README The quickstart no longer pins this repo's images, release binaries carry the defaults, so the regex rewrite had nothing to match and printed a misleading "updated README" line. Build, push, print. * gofmt internal/ate: #78 landed with space-indented code and no formatting check * ci: fail on unformatted Go (make verify-gofmt) * ate-env-guest: stop a sidecar's whole process group on shutdown runSidecar used exec.CommandContext, whose cancel kills only the direct child. A sidecar that forks (a wrapper script, which RUNTIMES.md recommends for quoted arguments) died alone while its child kept the output pipes open, and cmd.Wait blocked until that child exited. The guest's shutdown hung for as long as the orphan lived. TestRunSidecarRestartsAndStopsWithContext caught this on Linux and not on macOS: `sh -c "sleep 60"` is dash on Linux, which forks sleep, and bash on macOS, which execs it. First seen in the ci run on #71 (https://github.com/agent-substrate/env/actions/runs/38440900526), the first Linux run of this code. Each sidecar now runs in its own process group. On cancel the group gets SIGTERM, cmd.WaitDelay (sidecarStopGrace, 5 s) bounds the wait for the pipes, and the group gets SIGKILL for anything still alive. Verified in a golang:1.27 container, the test 3/3 and the guest packages green, and on macOS. (cherry picked from commit 93c5fb2) * workflows: pin the runner image to ubuntu-24.04 ubuntu-latest migrates to Ubuntu 26.04 from 2026-10-19 (actions/runner-images#14748). A release pipeline must not change its toolchain image underneath a tag; pin the image and bump it on purpose. * ate-env-guest: finish the sidecar stop sequence and cover it on every platform Review follow-ups on the process-group fix: - main waits for the supervisors after Serve returns (bounded by sidecarStopGrace plus a second), so the SIGTERM, grace and SIGKILL sequence runs before the process exits. Inside an actor the guest is PID 1 and the container ends with it anyway; outside one, this is what stops the sidecars. - The old process group is SIGKILLed before a restart as well, so a restarted sidecar does not compete with its own orphans for ports. - A stop is logged. - The test forks explicitly (`sleep 60 & wait`): a bare `sh -c "sleep 60"` execs on macOS and did not exercise the group kill there. The bound is derived from sidecarStopGrace and explained: the forked sleep holds the pipe, so returning within it proves the child died. (cherry picked from commit 922a95b) * Address review: name the worker-image publish command, anchor the cosign identity, strip CR before editing notes - The --worker-image message, the README quickstart and docs/release.md name `go run ./cmd/ate-setup publish worker-images` (which pushes the ateom images and prints their digest-pinned refs) instead of "build it with ko", and warn that a registry's :latest is whatever was built last and a stale worker image registers workers with no capacity. - The cosign identity regexp in the release notes and the docs is anchored to this repository's release workflow at a refs/tags/ ref, not to any identity under the repository. - The republish path strips carriage returns from the release body before looking for the previous "## Images" block; with CRLF from the API the block was appended twice instead of replaced. * ate-env-guest: one stop sequence for sidecar process groups, with step logs Review follow-up on #79. stopGroup is now the only way a sidecar's group is taken down: SIGTERM, up to sidecarStopGrace for the group to disappear, then SIGKILL for what is left. Both paths use it: after a cancel, where it finishes what cmd.Cancel started, and before a restart, where it clears what a crashed sidecar left behind so the new one does not compete with its own orphans for ports, pipes or lock files. Each step is logged: SIGTERM sent, group exited, still alive after the grace and SIGKILL sent, or already gone (ESRCH counts as success). main's shutdown wait covers both grace periods. The test asserts the logged sequence. (cherry picked from commit 8eb8274)

Tomer Glottmann (tomergee) · 2026-10-13T20:45:19Z

Landed in main through #71's squash (077ae95): #71 carried this branch's four commits as cherry-picks, and main's tree is identical to this branch's head 8eb8274 for every file this PR touches (cmd/ate-env-guest/{main,sidecar,sidecar_test}.go, internal/ate/client{,_test}.go). Nothing remains to merge, thus closing.

Tomer Glottmann (tomergee) added 2 commits October 13, 2026 11:35

gofmt internal/ate: #78 landed with space-indented code and no format…

ec682ef

…ting check (cherry picked from commit 86eebcc)

Tomer Glottmann (tomergee) requested a review from Vicente Ferrara (vicentefb) October 13, 2026 19:07

Tomer Glottmann (tomergee) closed this Oct 13, 2026

Tomer Glottmann (tomergee) deleted the fix/sidecar-stop-process-group branch October 13, 2026 20:46

Navigation Menu

Repository navigation

ate-env-guest: stop a sidecar's whole process group on shutdown - #79

ate-env-guest: stop a sidecar's whole process group on shutdown#79
Tomer Glottmann (tomergee) wants to merge 4 commits into
mainfrom
fix/sidecar-stop-process-group

Tomer Glottmann (tomergee) commented Oct 13, 2026 •

edited

Loading

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Sunny (sunnylovestiramisu) commented Oct 13, 2026

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Tomer Glottmann (tomergee) commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Testing

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Sunny (sunnylovestiramisu) commented Oct 13, 2026

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Tomer Glottmann (tomergee) commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Tomer Glottmann (tomergee) commented Oct 13, 2026 •

edited

Loading