refactor+fix: cleanup backlog — pkg/procutil, internal/fsutil, circuit breaker (#82/#86/#87)#93
Merged
Merged
Conversation
…rocutil deployFlake, lsofListener and killStrayProcessCompose were duplicated byte-for-byte across all four engine plugins' <engine>.go files, and lsofListener was duplicated a fifth time in internal/cli/status.go (as lsofListen). Hoist them into pkg/procutil — the non-docker sibling of pkg/dockerutil — so there is one copy to fix. DeployFlake takes the plugin's embedded assets as an fs.FS parameter (embed.FS satisfies it) instead of hardcoding a package-level nixAssets, so the one implementation serves every plugin. The package is split so the portable helpers (DeployFlake, LsofListener) carry no build tag and can be imported by the untagged internal/cli/status.go, while KillStrayProcessCompose (which needs syscall.Kill) stays in a darwin||linux file. This commit migrates mysql / postgres / redis and status.go; the elasticsearch migration rides with its heap/timeout hardening in a later commit (same file). Behaviour is unchanged — the extracted bodies are identical to the originals, now covered by pkg/procutil unit tests. Refs #82 (items 4, 5).
expandHome existed three times (internal/cli/ecc_import.go,
internal/registry/registry.go, internal/gitwt/gitwt.go) with three
different behaviours, and copyFile existed twice (an atomic tmp+rename
in ecc_import, a non-atomic ReadFile+WriteFile in registry).
Consolidate into internal/fsutil with one correct implementation of
each:
- ExpandHome adopts gitwt's behaviour (the most complete): a bare "~"
and "~/x" expand, "~user" is left untouched. This fixes registry
(which mishandled a bare "~") and ecc_import (which mangled
"~user").
- CopyFile standardises on the atomic tmp+rename and now also
preserves the source's mode bits (neither original did), upgrading
registry's previously non-atomic, mode-dropping backup copy.
ecc_import keeps its copyFile as a thin dispatcher — the .md
instinct-healing branch is domain logic — and delegates the generic
branch to fsutil.CopyFile.
Refs #87.
The daemon-lifetime limiter's circuit breaker never opened: tickOnce called limiter.Acquire() but runObserverOnceQuiet swallowed the pass exit status (`_ = c.Run()`), so RecordFailure / RecordSuccess were never called and the failure tally stayed at zero forever. A wedged or crashing extraction pass would keep firing indefinitely. runObserverOnceQuiet now returns the subprocess exit status, and tickOnce records it on the limiter so the breaker trips after CircuitBreakerN consecutive failures. A pass killed because the daemon is shutting down (ctx cancelled -> our Cancel SIGKILLs it) is NOT counted — a clean stop must not leave the breaker open. Refs #86.
…dening
elasticsearch:
- Reject a malformed engines[].extras.heap (heapSizePattern
^\d+[kmgKMG]?$) before it reaches the ES_JAVA_OPTS="-Xms${heap}..."
shell interpolation in the nix flake, closing a shell-injection seam
(#82 item 6).
- Bump the nix-backend graceful shutdown budget from 15s to 60s to
match the docker backend's dockerStopTimeoutSec — ES translog flush
is slow on both paths, so the 4x gap was inconsistent (#82 item 7).
- Extract the nix/process helpers to pkg/procutil (the elasticsearch
half of the item 4/5 de-dup, kept in this commit because it shares
the file with the heap/timeout changes).
CLI hardening:
- remove.go: log the pre_remove hook's error instead of swallowing it,
matching every other best-effort teardown step (#82 item 1).
- plugins_list.go: pathEnv reads os.Getenv("PATH") directly instead of
forking `sh -c` for a "fakeExec" test seam that never existed
(#82 item 3).
Refs #82 (items 1, 3, 5, 6, 7).
Up() validated the heap format after opening the startup log file, so an early return on an invalid heap leaked that file handle (the sibling cmd.Start() failure path a few lines later does close it). Validating before flakeDir/logPath setup fixes the leak at its root instead of adding another Close() call to remember. The docker backend never validated heap at all -- pickHeap's result went straight into the container env, so a malformed value only surfaced later as an opaque JVM/container startup failure. Both backends now share one validateHeap check.
Every earlier failure branch in CopyFile removes the temp file before returning; the final os.Rename did not, orphaning a fully-written temp file on disk whenever the rename itself failed (e.g. onto an existing non-empty directory).
Unifying the three expandHome implementations onto ExpandHome (which never errors) silently dropped ecc_import's prior behavior of propagating a UserHomeDir failure as an explicit error. With $HOME unset, `bough ecc import` continued with a literal, un-expanded "~/..." path and failed later with a generic "ECC root not found" message instead of the real cause. ExpandHomeStrict returns the same result as ExpandHome plus the UserHomeDir error when the input can't be resolved; ExpandHome is now a thin wrapper around it. ecc_import is the only caller that needs the strict variant -- registry and gitwt keep the silent-fallback behavior they already relied on.
ikeikeikeike
added a commit
that referenced
this pull request
Jul 6, 2026
The local main ref this branch forked from was stale — PR #91/#92/#93 were merged via `gh pr merge` (GitHub API) directly against the remote, per this session's standing rule to never touch the shared checkout's local main HEAD while a parallel session might be using it, but the local main ref was never fetched afterward. This branch was created from that stale local ref, so it never had PR #91's newly added "UpReuse" conformance phase (Up called a second time while the service is already running, must be a no-op) locally to test against.
ikeikeikeike
added a commit
that referenced
this pull request
Jul 6, 2026
Merging origin/main pulled in PR #91's new conformance UpReuse phase (Up called a second time while the service is already running, must be a no-op) -- this branch predates that merge locally, since #91/#92/ #93 were merged via `gh pr merge` directly against the remote without ever fetching the result back into this session's shared checkout, so this branch forked from a stale local main ref. That phase caught a real bug: Up() checked dockerutil.IsPortFree unconditionally, so a second Up() against an already-running container -- which legitimately holds the port -- was rejected as a "foreign" conflict instead of being reused. dockerutil.UpOrReuse now runs first (same order the mysql/postgres/redis/elasticsearch docker.go plugins already use): if a container named bough-compose-<port> is already running, Up reuses it and skips the port-conflict check entirely; IsPortFree is only consulted on a genuine fresh start. Sidecar state is now written unconditionally (reuse or fresh) since Down/EnvVars/ReadyCheck must work correctly even from a freshly-spawned plugin process that did not itself create the container. Not reproducible on this macOS dev machine: Docker Desktop's proxy layer does not visibly bind host ports the way native Linux does, so IsPortFree(port) returned true even with the container's own port already published, masking the bug locally. Confirmed fixed via the local conformance suite (UpReuse phase passing) after merging in the newer conformance package; the original failure was only caught by the ubuntu-24.04 CI runner.
This was referenced Jul 16, 2026
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
1 participant
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.
Closes #86, #87. Addresses #82 (items 1, 3, 4, 5, 6, 7).
Cleanup backlog from the retrospective review sweep:
deployFlake,lsofListener,killStrayProcessCompose) across all four plugins +status.go→ newpkg/procutil;expandHome(×3, three behaviours) andcopyFile(×2, atomic vs not) → newinternal/fsutilwith one correct impl each (atomic copy + mode-preserve; gitwt's~handling).runObserverOnceQuietswallowed the pass exit status, soRecordFailurewas never called. It now returns the status andtickOncerecords it (skipping shutdown-kills so a clean stop doesn't open the breaker).remove.gologs the pre_remove hook error), item 3 (pathEnvdrops ash -cfork for afakeExecseam that never existed), item 6 (validate ESextras.heapbefore the nixES_JAVA_OPTSshell interpolation), item 7 (align ES nix graceful timeout with docker's 60s).Not in scope: #82 item 2 (fixed in wave3), item 8 (fixed in #74), item 9 (SELinux
:Z— deferred: PLAUSIBLE and untestable on macOS / dev-linux; see the issue comment).Verified:
go test ./...(incl. new procutil / fsutil + circuit-breaker + heap tests) green;golangci-lint0 issues; mysql + elasticsearch docker conformance pass on the refactored binaries.