Add cloudemu lifecycle CLI: start/stop/status/logs/delete (#335 P0-a) - #336
Conversation
Add minikube-style background lifecycle to the cloudemu CLI, wrapping the existing foreground `serve` (issue stackshy#335, P0-a): - start [serve flags]: re-execs `serve` detached (new session, output → log), waits for readiness (HTTP health probe for AWS/GCP, TCP probe for the self-signed HTTPS endpoints), prints endpoints, and is idempotent. - stop: SIGTERM + wait for exit (reuses serve's graceful shutdown). - status: running/stopped + pid + endpoints. - logs [-f]: print/follow the daemon log. - delete: stop + remove the run directory. Run state (pid, log, resolved endpoints) lives under ~/.cloudemu by default, overridable with --home. serve is unchanged; start passes its flags through. Unit-tested the pure helpers (run-dir resolution, state round-trip, process-liveness, health poll, home-flag split); documented in docs/standalone-server.md. Persistence across restarts (stackshy#107) is the next step.
thzgajendra
left a comment
thzgajendra
left a comment
There was a problem hiding this comment.
Review — lifecycle CLI (start/stop/status/logs/delete), #335 P0-a
Solid, ship-worthy after a few fixes — no blockers. Purely additive, well-structured, serve untouched. go build, go vet, go test -race ./cmd/cloudemu/, and gofmt are all green locally, and golangci-lint is clean on the new lines. The strongest issues cluster around one theme: failure paths that leave an invisible, untracked daemon — worth closing before merge because the whole point of the feature is a managed instance you can reliably stop.
Fix before merge (all small, localized)
- Orphan daemon cluster (inline):
writeStatefailure returns an error while the daemon keeps running untracked; the readiness-abort SIGTERMs without waiting;stopnever escalates to SIGKILL after its 12s timeout. Each leaves a live process nostop/statuscan find. readStateconflates "absent" vs "unreadable" (inline): a corrupt/permission-deniedstate.jsonon a running daemon makesstop/statusreport it down — anddeletethen wipes the state, orphaning it permanently. One-line fix: branch onerrors.Is(err, os.ErrNotExist).deleteRemoveAll footgun (inline):os.RemoveAllon the verbatim--homedeletes the whole dir, not just cloudemu's files.
Should fix / follow-up
- PID-reuse safety —
processAliveis a bare signal-0 probe; a recycled PID meansstopcould SIGTERM an unrelated process. Store & verify identity (start-time / exe path / held lock file). - Windows (inline):
Setsidis Unix-only and there are no build tags; CI is Linux-only so it isn't caught, butdocs/standalone-server.mdadvertises barego install …@latest, so a Windows user gets a cryptic compile error.//go:build unixsplit + a graceful stub, or a doc note. - Concurrent
starthas no lock (O_CREATE|O_EXCLfixes it);logs -fsilently stalls after a restart truncates the log (re-seek when size < offset). - Readiness probe checks only the FIRST endpoint — fine as a proxy since
servebinds all listeners before writing the endpoints file, but the PR text "all 4 providers verified up" overstates what the code checks. Probe all present endpoints if you want the claim to hold.
Tests (coverage 9.2%)
The pure helpers are cheaply testable but skipped: waitServerReady (all 3 branches), pollTCP (twin of the tested pollHealth), readEndpoints (prune), waitForEndpoints (all-empty gate), httpHealthURL, hostPortOf, runLifecycle unknown-cmd, waitExit timeout, runStop stale-state, and runStart already-running (writable via a self-PID state). spawnServe/full runStop as manual-E2E-only is defensible, but the readiness-failure child-cleanup path is the highest-risk untested behavior — give it a harness test.
Docs (minor, while you're in the file)
- The doc shows the foreground banner ("standalone server", Title-case keys);
start/statusprintprintEndpoints("cloudemu — running", lowercase keys). Reconcile or show a realstartsample. - Ports table lists Kubernetes as HTTP; it's served HTTPS (and the new TCP-probe branch depends on that) — pre-existing, but you're editing the file.
- Nit:
os.FindProcesserr-check is dead code on Unix.
Great first slice of the minikube-ification roadmap — the structure (detached re-exec, no daemon lib) is the right call.
| Args: serveArgs, | ||
| } | ||
|
|
||
| return eps, writeState(dir, state) |
There was a problem hiding this comment.
[MED] Orphaned daemon on writeState failure. The child is already up and confirmed ready by this point, so returning (eps, writeState(...)) means a writeState error (disk full, perms) leaves a running but untracked daemon — no state.json, so stop/status can never find it; only pkill can. Roll back the child on failure:
if err := writeState(dir, state); err != nil {
_ = cmd.Process.Signal(syscall.SIGTERM)
return nil, fmt.Errorf("failed to persist daemon state (daemon killed): %w", err)
}
return eps, nilThere was a problem hiding this comment.
Fixed. spawnServe now kills and reaps the child on a writeState failure (killChild — SIGTERM, then SIGKILL after the timeout, then Wait) and returns failed to persist daemon state (daemon killed). No untracked daemon is left behind.
| } | ||
|
|
||
| if readyErr != nil { | ||
| _ = cmd.Process.Signal(syscall.SIGTERM) |
There was a problem hiding this comment.
[MED] Readiness-abort doesn't wait for the child. This fires SIGTERM and returns immediately — the child is Setsid-detached, so if it's slow to exit or ignores SIGTERM it lingers as an untracked live process holding the port (and no state was written). Bounded-wait + escalate:
if readyErr != nil {
_ = cmd.Process.Signal(syscall.SIGTERM)
if waitExit(cmd.Process.Pid, stopTimeout) != nil {
_ = cmd.Process.Kill()
}
return nil, fmt.Errorf("cloudemu failed to start (see %s): %w", logPath(dir), readyErr)
}There was a problem hiding this comment.
Fixed. The readiness-abort path now calls killChild(cmd) — bounded SIGTERM→SIGKILL with a reap — instead of a fire-and-forget SIGTERM. (Reaping matters here specifically: the CLI is still the child's parent at this point, so a bare signal would leave a zombie that signal-0 still reports alive — that's the exact failure a harness test now catches, TestKillChildReaps.)
| return err | ||
| } | ||
|
|
||
| if err := waitExit(s.PID, stopTimeout); err != nil { |
There was a problem hiding this comment.
[MED] stop never escalates to SIGKILL. If the daemon ignores SIGTERM, waitExit returns errTimeout after 12s and (per the flow below) removeState still runs — orphaning a live daemon that the CLI now can't track. After the timeout, proc.Signal(syscall.SIGKILL) + a short second waitExit before removing state.
There was a problem hiding this comment.
Fixed. stop now uses terminate — SIGTERM, and if the daemon hasn't exited within stopTimeout, SIGKILL + a second bounded wait — and only removes state once the process is confirmed gone. (terminate is the non-child variant: the target daemon is reparented to init, so it's reaped on death and signal-0 reports it gone.)
| return err | ||
| } | ||
|
|
||
| s, err := readState(dir) |
There was a problem hiding this comment.
[MED] readState conflates "absent" with "unreadable". A missing file, a permission error, and malformed JSON all collapse to "cloudemu is not running" here — so a corrupt state.json on a live daemon reports it down and it becomes unstoppable via the CLI (and runDelete, which calls this, then RemoveAlls the state, orphaning it for good). runStatus:451 has the same conflation. Fix:
if errors.Is(err, os.ErrNotExist) { fmt.Println("cloudemu is not running"); return nil }
if err != nil { return fmt.Errorf("reading state (daemon may still be running): %w", err) }There was a problem hiding this comment.
Fixed. Both stop and status now branch on errors.Is(err, os.ErrNotExist): a missing state is "stopped/not running", but a corrupt/permission-denied state.json returns reading state (daemon may still be running): … rather than silently reporting it down. delete therefore no longer wipes an unreadable-but-live daemon's state.
| return err | ||
| } | ||
|
|
||
| if err := os.RemoveAll(dir); err != nil { |
There was a problem hiding this comment.
[MED] RemoveAll footgun with --home. dir is the verbatim --home value, so delete --home /some/shared/dir wipes the entire tree, not just cloudemu's state.json/cloudemu.log/endpoints.json. Safer: remove the known files (removeState + os.Remove(logPath) + os.Remove(endpointsPath)) then os.Remove(dir) (succeeds only if now empty), or refuse RemoveAll when home != "".
There was a problem hiding this comment.
Fixed. delete now removes only cloudemu's own files (state.json, cloudemu.log, endpoints.json) and then os.Remove(dir) (which succeeds only if the dir is now empty) — never RemoveAll on a user-supplied --home.
| cmd := exec.CommandContext(context.Background(), exe, full...) | ||
| cmd.Stdout = logF | ||
| cmd.Stderr = logF | ||
| cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} |
There was a problem hiding this comment.
[LOW-MED] Unix-only, no build tag → doesn't compile on Windows. syscall.SysProcAttr{Setsid: true} (plus syscall.Signal(0)/SIGTERM) is Unix-only; GOOS=windows go build ./cmd/cloudemu/ fails here. CI is Linux-only so it isn't caught, but docs/standalone-server.md advertises bare go install …@latest, so a Windows user hits a cryptic error. Either split the platform bits into lifecycle_unix.go (//go:build unix) + a Windows stub that errors gracefully, or document Unix/macOS-only.
There was a problem hiding this comment.
Fixed. Split the daemon impl behind //go:build unix (lifecycle.go, lifecycle_test.go) and added lifecycle_other.go (//go:build !unix) whose runLifecycle returns a clear start/stop/... are only supported on Unix/macOS; run \cloudemu serve` instead. The command dispatch + names live in main.go(all platforms). Verified:GOOS=windows go build ./cmd/cloudemu/` now compiles.
- spawnServe: on a readiness failure OR a writeState failure, kill AND reap the child (new killChild) instead of a fire-and-forget SIGTERM, so a failed start never leaves an untracked detached daemon. - stop: escalate SIGTERM → SIGKILL after the timeout (via terminate) before removing state, so a signal-ignoring daemon can't be orphaned. - readState callers branch on os.ErrNotExist: a corrupt/unreadable state.json on a live daemon now errors instead of reporting "stopped" (which delete would otherwise wipe, orphaning it). - delete removes only cloudemu's own files (state/log/endpoints) + an empty-dir rmdir, never a blanket RemoveAll of a user-supplied --home. - PID-reuse guard: stop/start verify the recorded endpoints answer before signalling a live PID (portable identity check). - Windows: build-tag the daemon (//go:build unix) with a graceful non-Unix stub so `go install` no longer fails to compile off Unix. - readiness now probes every present endpoint; logs -f re-seeks on truncation. Adds unit tests for the pure helpers + killChild reaping + stop stale-state + start already-running (coverage 9% → 24%). Docs: Kubernetes protocol corrected to HTTPS.
|
Thanks for the thorough review — all six fix-before-merge items are addressed inline. On the rest: Should fix / follow-up
Tests — added unit coverage for the cheap pure helpers you listed ( Docs — corrected the Kubernetes protocol to HTTPS in the ports table, the banner sample, and the endpoints-file example. Full gate green after the changes: |
TestSDKConverseStreamMultibyteRuneBoundary intermittently failed in CI with "use of closed network connection". The runtime streaming handler already drains the request body, but a streamed (chunked) eventstream response left for keep-alive reuse can still be torn down abruptly as the handler returns, racing the client's in-flight read of the final events. Set `Connection: close` on the eventstream response (in newEventWriter, so both converse-stream and invoke-with-response-stream get it) so net/http ends the connection with a clean FIN after the last flush instead of attempting reuse. Unrelated to the lifecycle CLI in this PR, but surfaced by its pipeline run.
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Deep review — lifecycle CLI
Nice work — this is careful, well-structured code. Verified in an isolated worktree at the PR head: CI gates all green (build / vet / go test -race ./cmd/cloudemu/ ok / gofmt clean), the two-phase readiness is sound (serve binds every listener before writing the endpoints file, so "file exists ⇒ ports accept," then waitServerReady confirms), the unix/non-unix build-tag split is clean, killChild (reap our child) vs terminate (signal-0 on the reparented daemon) is the right distinction, and delete removes only cloudemu's own files rather than RemoveAll-ing a user --home. Docs are updated and even fix the pre-existing k8s HTTP→HTTPS port error. Mirror rule is N/A (ops/CLI feature).
Holding on one real functional bug (M1) plus a few low notes.
M1 (Medium) — cloudemu start --admin=false kills a healthy server
The AWS/GCP readiness probe hits /_cloudemu/health, but that route returns 200 only when the admin control plane is mounted — server/admin/admin.go:96, and handlerFor mounts it only when c.admin is true (serve.go:234). start forwards all serve flags through (this PR advertises "all serve flags pass through"), so cloudemu start --admin=false — which includes the default all-providers run — starts serve fine, but the probe never sees 200, waitServerReady times out (~15s), and killChild then kills a perfectly healthy server, reporting "failed to start." Confusing failure on a documented, realistic flag (hardening the emulator by disabling the state-wiping admin plane). The same admin dependency also sits in daemonReachable (idempotent-start / stop's stale-check); that path is unreachable today only because start never manages to record an --admin=false daemon. Inline on the probe line. Fix: make the aws/gcp probe admin-independent (TCP-accept like azure/k8s, or fall back to TCP when /health isn't 200).
L1 (Low) — gosec G703 ×3 on --home (version drift)
os.MkdirAll/os.Remove on a --home-derived path (lifecycle.go:71,384,588). In a CLI the user supplies their own --home — not a trust boundary, so it's a false-positive vuln. G703 taint analysis is newer gosec; with golangci-lint 2.11.4 it fires, which explains why your --new-from-rev run reported 0 (linter version drift). Not CI-blocking (CI doesn't run golangci-lint), but the local gate per CLAUDE.md wants 0 — please confirm against the repo's pinned linter version; if it fires there, a //nolint:gosec // user-supplied --home, not a trust boundary closes it.
L3 (Low) — no automated test for the spawn happy path
The pure helpers are well covered, but the actual detached-serve spawn (spawnServe/runStart happy path) is only in the manual e2e — and an e2e over --admin=false is exactly what would have caught M1. Optional: one test that spawns the built binary and asserts start→status→stop.
No AI attribution. Requesting changes on M1; the Lows are cheap follow-ups.
|
|
||
| for _, k := range []string{"aws", "gcp"} { | ||
| if ep := eps[k]; ep != "" { | ||
| if err := pollHealth(strings.TrimRight(ep, "/")+"/_cloudemu/health", timeout); err != nil { |
There was a problem hiding this comment.
M1 (Medium). This probes /_cloudemu/health, which only returns 200 when the admin control plane is mounted (server/admin/admin.go:96; handlerFor gates it on c.admin, serve.go:234). Since start forwards every serve flag, cloudemu start --admin=false boots serve fine but this probe never sees 200 → waitServerReady times out (~15s) → killChild kills a healthy server → "failed to start." Make the aws/gcp readiness check admin-independent: TCP-accept probe like azure/kubernetes, or fall back to pollTCP(hostPortOf(ep)) when /_cloudemu/health doesn't return 200. (Same admin dependency is in daemonReachable.)
There was a problem hiding this comment.
Fixed. Readiness is now an admin-independent TCP-accept probe for every provider — waitServerReady (and daemonReachable) call pollTCP(hostPortOf(ep)) instead of hitting /_cloudemu/health. Since serve binds every listener before writing the endpoints file, "accepts a connection" is a sufficient readiness signal and no longer depends on the admin control plane. pollHealth/httpHealthURL are removed.
Verified E2E: cloudemu start --admin=false --providers aws --aws-port 4577 now returns exit 0 with the server up (/_cloudemu/health → 404, but status/stop work); previously it timed out and killChild killed the healthy server. Added a regression guard TestWaitServerReady case: a bare TCP listener under the aws key (no HTTP handler, i.e. the --admin=false shape) is seen as ready. 2237be6
| } | ||
| defer logF.Close() | ||
|
|
||
| full := append([]string{"serve", "--endpoints-file", epPath, "--quiet"}, serveArgs...) |
There was a problem hiding this comment.
L2 (Low). You inject --endpoints-file <epPath> --quiet then append user args. If a user passes cloudemu start --endpoints-file /x, Go's flag parser is last-wins → serve writes to /x, so waitForEndpoints(epPath) waits on a file that's never written and times out (~15s) with a confusing "failed to start." Strip/reject a user-supplied --endpoints-file the way you strip --home (and optionally dedupe --quiet).
There was a problem hiding this comment.
Fixed. start now strips any user-supplied --endpoints-file (with its value) and --quiet from the forwarded args before injecting its own, via stripFlag(rest, "endpoints-file", true) / stripFlag(rest, "quiet", false) — so last-wins parsing can't redirect serve's output away from the run dir or duplicate the flag. start owns both flags.
Verified E2E: cloudemu start ... --endpoints-file /tmp/should-be-ignored.json --quiet starts normally and never creates that file. Added TestStripFlag (space-separated and = forms). 2237be6
…/L2) Readiness/reachability now use a plain TCP-accept probe for every provider instead of an HTTP /_cloudemu/health check. /health is served only when the admin control plane is mounted, so `cloudemu start --admin=false` used to boot a healthy server the probe never saw, time out, and kill it. serve binds every listener before writing the endpoints file, so TCP-accept is a sufficient and admin-independent readiness signal. Also strip user-supplied --endpoints-file/--quiet in `start`: start manages both, and Go's last-wins flag parsing let a forwarded --endpoints-file redirect serve's output away from the run dir, breaking the readiness handshake.
|
Thanks for the deep review. Pushed M1 (fixed) — L2 (fixed) — a forwarded L3 (addressed) — added the L1 (won't-fix, with reason) — gosec G703 on the Gate: |
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Approving — M1 and L2 resolved
Verified the fix (2237be62) in an isolated worktree at the PR head; gate green (build / vet / go test -race ./cmd/cloudemu/ ok / gofmt clean, no dangling refs to the removed helpers).
M1 (the real bug) is fixed correctly. Readiness and daemonReachable now use a plain TCP-accept probe for every provider — the /_cloudemu/health HTTP dependency is gone. That's sound: serve binds every listener before writing the endpoints file, so "accepts a connection" is a sufficient, admin-independent ready signal, and cloudemu start --admin=false no longer boots a healthy server the probe can't see and kills. Nice touch adding TestWaitServerReady as an explicit regression guard for the no-admin case.
L2 is fixed — stripFlag drops a user-supplied --endpoints-file/--quiet before forwarding (covering --flag value, --flag=value, and single-dash forms), backed by TestStripFlag, so last-wins flag parsing can no longer redirect serve's output and break the handshake.
On L1 (gosec G703): confirmed non-blocking — it's linter-version drift (newer bundled gosec taint analysis), there's no gosec config opt-in in .golangci.yml, and CI doesn't run golangci-lint. It's a false-positive on the user's own --home flag; if a pinned linter ever surfaces it, a //nolint:gosec closes it. Not a merge concern.
Clean, well-tested turnaround — thanks. LGTM.
2 participants
Objective / Issue
First deliverable of #335 ("minikube for cloud resources"), P0-a: give cloudemu a background lifecycle so you can leave it running, instead of only the foreground
cloudemu serve.What we found
cloudemu serveruns in the foreground only — there was nostart/stop/status. Everything needed to build on already exists:servewrites an endpoints file, supports--endpoints-file/--quiet, has graceful SIGTERM shutdown, and exposesGET /_cloudemu/health.How we fixed it
New
cmd/cloudemu/lifecycle.go+ a dispatch inmain.go:start [serve flags]— re-execs this same binary asservedetached (new session viaSetsid, stdout/stderr → log file), waits for readiness, records run state, prints endpoints. Idempotent (a secondstartreports the already-running instance). Allserveflags pass through.stop— SIGTERM the pid and wait for exit (reusesserve's graceful shutdown), clean up state.status— pid-liveness + endpoints.logs [-f]— print/follow the daemon log.delete— stop + remove the run directory.Run state (pid, log, resolved endpoints) lives under
~/.cloudemu/by default;--home <dir>relocates it.serveitself is untouched.Readiness probe: plain-HTTP
/_cloudemu/healthfor the AWS/GCP endpoints; a TCP-connect probe for the self-signed HTTPS endpoints (Azure/Kubernetes) — so no TLS verification is ever disabled.Alternatives not taken
servekeeps it dependency-free and portable.start. That's P0-b (Add Time Travel with State Snapshots and Fork #107) and its own PR; documented under "Not yet included".Docs / Tests
docs/standalone-server.md— new "Background mode (start/stop/status)" section.lifecycle_test.go, TDD-first): run-dir resolution, state round-trip,processAlive, HTTP health poll (ready + timeout), and--homeflag extraction/passthrough.awsCLI v2.30:cloudemu start(all 4 providers) → every endpoint verified up.aws s3 mb/cp/ls,aws dynamodb create-table/list-tables,aws ec2 run-instances/describe-instances— all succeed against:4566.status(running) → idempotentstart→stop(ports released) →status(stopped) →delete(run dir removed); no stray processes.Test plan
go build ./...,go vet ./cmd/cloudemu/,gofmtgo test -race ./cmd/cloudemu/golangci-lint run --new-from-rev=$(git merge-base HEAD stackshy/development) ./cmd/cloudemu/...= 0Risk & Rollback
Low — purely additive (new subcommands + a new file);
serveand all in-process APIs are unchanged. Rollback = droplifecycle.goand themain.godispatch case.Follow-ups
stop→startkeeps resources.