Repository navigation
Fix engine detection, shutdown and test process hygiene - #56
Merged
Merged
Conversation
JaredP94
force-pushed
the
fix/engine-manager-hygiene
branch
from
November 6, 2026 11:31
d51a12e to
8674a10
Compare
Require the app CLI target as well as a declared launcher. Missing installations keep saved ON intent but are skipped at launch, leaving Install available in desktop and TUI without modifying user files. Bump engine-manager from released 1.4.0 to 1.4.1 for these fixes. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
A control CLI can close its listener before the serving process finishes tearing down. Capture that PID before stop and require its exit as well as consecutive connection refusals within the existing stop grace, without holding the state mutex. Preserve running state on timeout rather than allowing immediate restart to race pending shutdown. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
Use Ollama v0.40.15, the newest published release, for every bundled platform. The five artifact hashes come from its GitHub release sha256sum.txt and match fresh downloads independently hashed without execution. Initial installation is pinned; vendor self-updates are unchanged. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
Give every fake invocation and copied executable a per-test owner, with early PID registration inherited by detached children. Test cleanup kills owned process groups and checks for survivors, and TestMain fails and sweeps any missed registered children or launchers under this run directory. Identity checks keep sweeps scoped to this run. Exercise both fatal-test cleanup and the final missed-cleanup safety net. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
FreeBSD compilation had no fileIdentity or watchDownloadExit implementation. Fail closed for install identity and use ordinary reaping on unsupported targets, without claiming engine or platform support. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
JaredP94
force-pushed
the
fix/engine-manager-hygiene
branch
from
November 6, 2026 11:59
8674a10 to
9a6dbbb
Compare
Settings rollback replaces the entire runtime while a canceled health goroutine may still be starting. Read and copy its probe under the same state mutex used by rollback, then release the mutex before timers or network probes. Add a concurrent runtime-restoration regression that exposes the race on both main and the original PR head. The existing rollback test passed 50 race-enabled repetitions on both revisions; after the fix it still passes 50 repetitions, as does the new regression. Two uncached full race suites and darwin, linux, windows, and freebsd vet checks pass. Keep the PR's existing engine-manager version bump. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
…ders Settings rollback replaces Runtime under the state mutex while an already admitted passive status probe can still be detecting executables. Revision fencing prevents stale publication but does not synchronize those reads. Add engineState.platformSnapshot and use it for passive status/Detect, CLI and remove-path actions, profile residency observations, health startup and live-platform capabilities. Move capability filesystem probes outside mu. The copy is shallow: Detect, DetectRequires, Install, Uninstall, and all Runtime slice/map/pointer referents (Ready, Health, Stop, EditableLaunch, Args, Env, Start, LaunchArgs and LaunchEnv) are immutable after publication. Settings replace argument/environment pointers or Runtime as a whole; SetPort replaces only Port. installDir/modelsDir are fixed at construction. Test fixtures that customize referents do so before concurrent consumers. Audit of direct platform readers retained with their existing locks: - st.mu: platformSnapshot; refreshPassiveStatus's Ready and mode captures; reconcilePresence's Ready capture; stopEngines's shutdown mode check; launchStateLocked's ServerPort capture; SetPort's platform nil checks and Port restoration/update; ConfigureLaunch's apply/rollback writes. - st.opMu: Install's mode and Install reads; Uninstall's Uninstall/mode reads and installCreates(st.plat); doStart's port/Runtime reads and controlCLIPath(st.plat); doStop's Runtime capture; runCommandStop's controlCLIPath(st.plat); reconcileFailedCommandStop's Runtime capture; reconcilePresence's mode check; runLMSLoad's controlCLIPath(st.plat) (called by locked lmsModelAction or doStart -> restoreLMSModels); launchForState's Runtime; launchStateLocked's editable-launch and adopted-policy checks; previewLaunchLocked's port/Runtime; and ConfigureLaunch's previousRuntime, stopAttempt Runtime and persistence comparisons. All settings apply/rollback paths hold the same opMu. - state initializes from a local Platform copy before publication under Executor.mu. Registry PlatformFor/resolveEngine/capabilitiesFor consume the immutable registry, not engineState's replaceable Runtime. Readiness uses the Runtime captured by doStart under opMu; its cancellation watcher reads no platform. Process-output callbacks retain that private Runtime copy; exit watchers and process-stop goroutines read no platform. Command-stop cleanup remains synchronous under opMu. All three production health launch sites check Health first, and settings updates preserve that immutable pointer, including rollback. runHealth additionally snapshots and returns safely for nil Health instead of relying on the caller invariant. TestPassiveStatusConcurrentRuntimeRestore reproduces the status and Detect races on 7fea529 under -race. TestHealthStartupWithoutProbe reproduces the previous nil dereference. Keep TestSettingsApplyRestoresPreviousState and TestHealthStartupConcurrentRuntimeRestore unchanged. Validate the four regressions with -race -count=30, run the full race suite twice without the test cache, and vet darwin/linux/windows/freebsd. No additional version bump: the PR already contains the engine-manager bump. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
2 tasks done
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.
Description
Fix five engine-manager hygiene problems found during 0.9.0 validation:
runtime.stop.wait_for_exit, also confirming exit of the serving PID before restart; LM Studio's persistent host can remain alive after its HTTP server stops. The state mutex is not held across I/O or waits; existing lifecycle serialization is retained.Bump engine-manager 1.4.0 → 1.4.1 relative to released 0.9.0. These are fixes, not a new service interface.
Ollama checksum provenance
All checksums are published in https://github.com/ollama/ollama/releases/download/v0.40.15/sha256sum.txt:
Downloaded all five artifacts and sha256sum.txt into a fresh empty directory under /tmp. Independently hashed every artifact with shasum -a 256 and Python hashlib; all match the published file. Nothing downloaded was executed or installed.
Scope
Only services/nvpair-engine-manager, its manifests/README, services/versions.json and the two fixed known-issue entries. No desktop, shared library, peer-address, product changelog or supported-platform changes. Six focused signed-off commits, including the review follow-up. User engine directories and installed apps were not touched.
Validation
Local macOS arm64, Go 1.27.3:
cross-process-ubuntu-latestwithgh run downloadinto a new empty/tmp/pair-em-ci.*directory.python3 -I /tmp/pair-review/scripts/noskip.py <dir>/cross-process.jsonlreturned 122 pass, zero skips, zero failures.Go modules (macos-15)andCross-process tests (macos-15)never acquired a runner (empty runner name, zero steps). GitHub cancelled them after multiple acquisition attempts. Their annotations say: “The job was not acquired by Runner of type hosted even after multiple attempts” and report macOS arm64 capacity constraints. No test step failed; the overall run is unsuccessful due to these infrastructure cancellations. Local macOS module tests/race passed, but do not replace missing CI coverage.Regression evidence before fixes:
Review follow-up —
8674a106a5100280793338e4e77dfb9984da4665runtime.stop.wait_for_exit(boolean, default false), enabled only for bundled oMLX. Validation requires command mode, a stop command, and a readiness probe. Documented in MANIFEST.md and README.md. Existing deadlines and state-lock boundaries are unchanged, including failed-start cleanup.TestCommandStopAndRestartWithPersistentHostfailed on d51a12e with the listener closed and the host alive; passes with the fix, including start after stop and Restart on the same host PID.TestCommandStopWaitsForProcessExitAfterPortClosesandTestCommandStopRetainsRunningOnTeardownTimeoutretain explicit opt-in coverage;TestValidateStopWaitForExitcovers accepted/rejected declarations andTestBundledStopWaitForExitchecks bundled policy.go test -race -count=1 ./...passed before rebase (179s) and after rebase (183s). Fullgo test -count=1 ./...passed after rebase (177s). Vet passed for darwin/linux/windows/freebsd; FreeBSD build passed. gofmt output empty, diff check clean, SPDX zero missing, andpgrep -lf fake-enginefound no survivors.8674a106a5100280793338e4e77dfb9984da4665: four executed jobs passed — Desktop checks, SPDX headers, Go modules (ubuntu-latest), and Cross-process tests (ubuntu-latest). Downloadedcross-process-ubuntu-latestwithgh run downloadinto fresh empty/tmp/pair-pr56-ci-8674a106.iTtcwc;python3 -I /tmp/pair-review/scripts/noskip.py /tmp/pair-pr56-ci-8674a106.iTtcwc/cross-process.jsonlreports 123 passes, zero skips, zero failures.Go modules (macos-15)andCross-process tests (macos-15)never acquired a runner and were cancelled by GitHub at 11:46:43 UTC. Both have empty runner names and zero steps. Annotations state “The job was not acquired by Runner of type hosted even after multiple attempts” and cite macOS arm64 capacity constraints. Overall run conclusion is failure due to those infrastructure cancellations, not a failed test step. Local macOS unit/race results do not replace unexecuted CI coverage.Risk
Process-exit confirmation is manifest-declared and enabled only for bundled oMLX. Other engines retain listener-only confirmation, so LM Studio stop/restart does not wait for its persistent host. For opted-in engines, PID lookup is best-effort: if the host cannot resolve the serving PID, stop can confirm only transport closure. Lookup remains bounded by existing platform tool timeouts; stop CLI uses max(grace_s, 5s), then shutdown confirmation uses grace_s (default 5s). A timeout does not falsely mark stopped. The lifecycle serialization lock remains in place; no state mutex is held during waits.
The Ollama pin controls only a fresh install and is not runtime artifact certification or a block on vendor self-updates. No real engine install or live-engine smoke test was performed. FreeBSD is compilation-only, not runtime support.
Checklist