Repository navigation
Conversation
Extract the download job ownership and assignment gate into one helper for downloads and engine processes. Exercise assignment ordering, activation failures, termination, and idempotent handle closure through injected job operations. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
Assign servers, action CLIs, and install helpers to kill-on-close jobs before releasing their start gates. Use the same ownership helper as downloads and terminate jobs instead of taskkill PID trees. Keep command-mode vendor daemons independent by assigning their suspended control CLIs to CLI-only jobs, preserving bounded official stop commands. Bound command execution and output drainage, close jobs on exit and shutdown, and pin and revalidate legacy orphan handles before reclaiming them. Preserve Unix process behavior. Document the ownership boundary and add injected-job lifecycle and error-path regression tests. Windows runtime behavior remains untested. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
Keep a query and terminate handle open from listener identification through orphan cleanup instead of reopening a PID after an image-only lookup. Record the process creation time and fail closed unless the same handle still has that identity and image and its PID still owns the managed listener. Route Windows test-process cleanup through the same pinned identity checks, including run ownership validation for copied fixtures. Preserve Unix orphan signal semantics. Add injected regressions for same-image PID replacements on another port and on the same port with a different creation time, plus fail-closed and handle-close coverage. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
Review follow-up. Three test cleanups reopened a saved fixture PID with os.FindProcess and killed it unconditionally, so a PID the OS had since reused for another program could be killed. They now go through killOwnedTestPID, which kills only while the PID still runs that test's own fake-engine copy (a path unique to the test); on Windows the kill then revalidates ownership through a pinned handle. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
…nups Review follow-up. Cleanups in the e2e shutdown, command-mode StopAll and command-stop tail tests signalled a PID after a background Wait could already have reaped it, or read it back from a file. They now use killOwnedTestPID with the test's own fake-engine copy. The remaining direct kills run before the test waits on its child, so the PID cannot have been reused. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com>
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
Replace engine-manager's Windows PID-tree shutdown with owned kill-on-close jobs, sharing the downloader's gated assignment mechanism. This prevents owned descendants escaping cleanup by reparenting and avoids PID-reuse races during forced shutdown.
Scope
lms server stop/omlx stoppaths and listener/exit confirmation remain authoritative; no forced cleanup targets vendor daemons.Validation
On Darwin arm64, in
services/nvpair-engine-manager:go test -race -count=1 ./...— PASS (184.909s).go test -count=1 ./...— PASS (179.035s).go vet ./...andGOOS=linux,GOOS=windows(amd64 and arm64),GOOS=freebsdgo vet ./...— PASS.GOOS=windows GOARCH=amd64 go test -candGOOS=windows GOARCH=arm64 go test -c— PASS; output binaries kept outside the repository.gofmt -l .— empty;git diff --check— clean.node scripts/spdx-headers.mjs— 0 missing/review/unclassified.pgrep -lf fake-engine— no survivors.TestProcessJobAssignmentBeforeResume,TestProcessJobActivationFailureClosesWithoutResume,TestProcessJobCancellationBeforeActivationDoesNotResume,TestProcessJobStopAndCloseAreIdempotent,TestProcessJobTerminationErrorStillCloses; real-child ownership-lifetime testsTestJobCommandExitClosesJob,TestJobCommandStartFailureClosesJob,TestJobCommandAssignmentFailureReapsAndCloses,TestJobCommandShutdownTerminatesAndClosesJob,TestJobCommandDeadlineTerminatesJob, and explicit ownership marker coverageTestControlCommandOwnershipIsExplicit.canceled launch was resumedbefore the fix; removing termination-error fallback made its test fail with[terminate]instead of[terminate close]. Both pass with the fixes restored. The first full race run exposed a test-helper timeout caused by the race runtime's exit drain; the successful-exit fixture now has a ten-second bound, and both complete suites were rerun successfully.CI run 42240940318 on head
113eed2095263ac9c56809bc0bcbda77bd5848a1:runner_id: 0, empty runner names, and zero steps. This is unavailable CI coverage, not a passing macOS result; the local Darwin suite above did run successfully.cross-process-ubuntu-latestwithgh run downloadinto a new empty directory/tmp/pair-win-jobs-ci-dOTYhMand ranpython3 -I /tmp/pair-review/scripts/noskip.py /tmp/pair-win-jobs-ci-dOTYhM/cross-process.jsonl:{'pass': 123} [], exit 0, zero skips.Risk
Windows runtime behavior is untested. Cross-compilation, vet, and injected-job tests do not validate real Windows job nesting, console behavior, installer behavior, or vendor daemon creation. A Windows runtime smoke test remains necessary before claiming runtime certification.
Command-mode control jobs intentionally permit all their children to remain vendor-managed; this is narrower ownership than server/download/helper jobs and is required to preserve independent daemons. Legacy orphan cleanup now terminates only its proven process handle, not unproven descendants. Ordinary Windows commands also have a fifteen-minute safety ceiling in addition to existing caller deadlines and one-second output-drain bounds. Unix retains its existing SIGTERM behavior.
Checklist
git commit -s).