engine plugins: backend detection is heuristic, not tracked — risks datadir deletion + collateral kill of sibling engines

[ikeikeikeike]

Surfaced by the wave-2 retrospective /code-review sweep of PR #2/#3/#4/#6 (postgres/redis/elasticsearch providers + the shared Docker backend). Two related gaps in how a running engine's plugin process is identified at teardown time; this sweep applied a narrow mitigation for the first but the underlying design gap remains for both.

1. usingDockerBackend() guesses the backend from container existence, not from what Up() actually used

DownReq and ReadyCheckRequest carry no backend field (see plugins/engine/api/proto/*.proto — only UpRequest has extras/backend). So Down()/ReadyCheck() in all four plugins (mysql/postgres/redis/elasticsearch docker.go) reconstruct "was this docker or nix" by calling usingDockerBackend(), which just checks "does a container with this name exist and is it running".

This sweep tightened the check to also require ContainerInspect(...).State.Running (previously any container, including a long-stopped one, counted), which closes the most concrete false-positive. But the deeper issue remains: a transient Docker daemon error during Down() (dockerutil.NewClient() or LookupByName() failing) makes usingDockerBackend() return false even when the engine really is docker-backed, so Down() falls through to the nix-style lsof+SIGTERM path instead of dockerDown() — the container is never stopped/removed, and a later Cleanup() (rm -rf the bind-mounted datadir) can then race a still-running container process.

Proper fix: extend UpRequest/DownRequest/ReadyCheckRequest (proto regen required) with an explicit backend field the host persists (e.g. in the registry, alongside the allocated ports) and threads through every subsequent RPC for that engine instance, instead of re-derived heuristically per call.

2. killStrayProcessCompose can't tell engines within the same worktree apart

All four plugins' nix-backend Down() ends with killStrayProcessCompose(req.WorktreeRoot): pgrep -f process-compose + a cwd-prefix match, SIGTERM every match. But internal/cli/create.go's startEngines computes one engineProviderWorktree value per bough create call and passes it as WorktreeRoot to every declared engine — so a worktree running mysql+redis+es together (this project's own documented topology, see CLAUDE.md) has all three engines' nix run ... up processes sharing the identical cwd.

This sweep fixed the separate cross-worktree false-positive (.../auba-api-139 matching as a prefix of .../auba-api-1394) by anchoring the prefix match to a path-separator boundary. That fix does not help here — the cwd is byte-for-byte identical across engines in the same worktree, so tearing down any one engine's Down() call SIGTERMs the still-running process-compose supervisors of every other engine in that same worktree too, bypassing their own graceful shutdown.

Proper fix: give each engine's nix run invocation a way to be identified individually — e.g. a pidfile written at Up() time to a per-engine-kind path (<engineProviderWorktree>/.local/<kind>-process-compose.pid), with killStrayProcessCompose reading and validating that specific PID instead of a heuristic cwd sweep. This needs design sign-off since it changes the stray-cleanup contract all four plugins share.

Files

• plugins/engine/{mysql,postgres,redis,elasticsearch}/docker.go (usingDockerBackend)
• plugins/engine/{mysql,postgres,redis,elasticsearch}/*.go (killStrayProcessCompose)
• plugins/engine/api/proto/*.proto, internal/cli/create.go (startEngines)

[ikeikeikeike]

Found the same root cause in a different place: wave-3 review of PR #12 (bough status's backend column) found computeEngineBackends in internal/cli/status.go has the identical problem as this issue's #1 — it calls backend.Detect() once per bough status invocation and stamps that single, momentary result onto every worktree's row of a given engine kind, because (as this issue already says) no per-worktree backend choice is persisted anywhere to read back.

Concretely: two worktrees of the same engine kind that were created under different host conditions (e.g. one before Docker was installed, one after) will both show whatever Detect() returns right now — which can be flat wrong for one of them. bough status's own doc comment claims the field "matches the same Detect() the create path runs, so operators can spot-check without re-running bough create" — that claim only holds when a per-worktree backend choice actually exists to check against.

Applied a narrow mitigation in the wave-3 sweep: status.go now also honors extras.backend (previously only eng.Backend), and the doc comment on statusEntry.Backend now says explicitly that the auto-detect case is a live host guess, not a per-worktree readback. The actual fix (persist each worktree's chosen backend at bough create time, e.g. in the registry) is the same architecture change this issue already proposes for usingDockerBackend/DownReq/ReadyCheckRequest — bough status would consume the same persisted value once it exists.

[ikeikeikeike]

Two more concrete bugs in the same killStrayProcessCompose cwd-matching logic (now consolidated in pkg/procutil/kill_unix.go as of PR #93), surfaced by a /deep-investigate audit of that PR. Both compound the case for item 2's proposed pidfile fix, since a pidfile-based lookup would sidestep both of these too rather than needing yet another string-comparison patch.

3. cwdPrefix is never symlink-resolved, but cwd (from lsof) always is

pkg/procutil/kill_unix.go:45:

if cwd := fields[len(fields)-1]; cwd == cwdPrefix || strings.HasPrefix(cwd, cwdPrefix+"/") {

cwd comes from lsof's NAME column, which reports the kernel-resolved real path. cwdPrefix is req.WorktreeRoot, built by plain filepath.Join in internal/cli/create.go with no filepath.EvalSymlinks anywhere in the chain. On stock macOS, /tmp → /private/tmp and /var → /private/var are symlinks; a worktree living under either has lsof report /private/tmp/... while cwdPrefix stays /tmp/..., so the comparison never matches — the stray process-compose supervisor is never SIGTERM'd, and it respawns the just-killed engine. (internal/gitwt/gitwt.go already resolves this exact class of mismatch via EvalSymlinks for a different comparison — the fix pattern already exists in the codebase, just not applied here.)

4. A space anywhere in the worktree path breaks the whitespace-split parse

pkg/procutil/kill_unix.go:39:

fields := strings.Fields(scanner.Text())

splits the raw lsof -p <pid> line on arbitrary whitespace, then line 45 takes fields[len(fields)-1] as the whole cwd. A cwd containing a space (e.g. /Users/Jane Doe/projects/...) splits the NAME column into extra fields, so the "cwd" bough extracts is only the last path segment, not the real cwd — breaking the same match as #3.

Neither is exercised by pkg/procutil/exec_unix_test.go's only test (TestKillStrayProcessCompose_NoMatchIsSafe), which uses a nonexistent prefix and never asserts a real match.