Skip to content

[3/3] fix: own Windows process trees with native job supervision - #458

Open
slashdevcorpse wants to merge 12 commits into
LodyAI:mainfrom
slashdevcorpse:fix/windows-job-process-ownership
Open

[3/3] fix: own Windows process trees with native job supervision#458
slashdevcorpse wants to merge 12 commits into
LodyAI:mainfrom
slashdevcorpse:fix/windows-job-process-ownership

Conversation

@slashdevcorpse

@slashdevcorpse slashdevcorpse commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Related issue

Refs #429

Problem / pressure

Windows agent processes and detached descendants can survive parent exit or crashes. Retained child handles make root termination safe, but durable descendant ownership requires a job established before target code starts.

Summary

  • Launch agent processes inside an unnamed kill-on-job-close Windows Job Object before target code executes, using suspended creation, atomic job assignment and a two-stage owner handshake.
  • Close the private job on owner death, control-channel withdrawal, supervisor failure or target exit, terminating owned descendants.
  • Pass an absolute Node executable to the supervisor; the bundled Node launcher preserves PATH/.cmd resolution, structured arguments, standard streams, working directory and target environment. Missing ownership assets fail closed.
  • Integrate ownership with ACP runners, authentication and sandbox launches. Include native assets and launcher in CLI/Electron packaging.
  • Build the supervisor after each Vite output, including watch builds. Populate the selected MSVC/SDK tool PATH explicitly. Gate compiler-dependent integration tests behind LODY_WINDOWS_SUPERVISOR_INTEGRATION=1 and give setup the complete native-build timeout budget.
  • Build/test x64 and ARM64 in Windows CI. A dependent Linux job consumes both same-run artifacts after building JavaScript, runs prepack without publishing, and verifies both executables plus the launcher in the npm archive.

Dependency order

Windows process lifecycle series for #429, in bottom-to-top order:

  1. [1/3] fix: bound Windows ACP shutdown and retain failed cleanup #456 — bounded shutdown and retained cleanup ownership.
  2. [2/3] fix: protect background sessions and record resource history #457 — background-aware eviction and resource history; depends on [1/3] fix: bound Windows ACP shutdown and retain failed cleanup #456.
  3. [3/3] fix: own Windows process trees with native job supervision #458 — native Windows Job Object ownership; depends on [1/3] fix: bound Windows ACP shutdown and retain failed cleanup #456 and [2/3] fix: protect background sessions and record resource history #457.

All three PRs are submitted to LodyAI/Lody. Their source branches have linear ancestry. Because GitHub does not support native cross-fork stacks, each PR targets upstream main and later PRs include predecessor changes until those land.

Review the individual layers:

The complete series is required for Windows descendant and crash ownership. The former fork PRs slashdevcorpse#2, #3 and #4 are superseded by this upstream series.

Test plan

  • Native Windows x64 ownership and packaging tests: 16/16 passed. Earlier negative controls disabled kill-on-job-close and correctly detected surviving synthetic descendants.
  • At rebased head 8a53cfd, 124 focused composed-stack tests and all five native launcher integration tests passed. The composed test pins five original OS handles, makes the supervisor PID getter throw, terminates through the retained child handle and verifies all five exited. A bare PATH-only .cmd test confirms launcher resolution and argument preservation.
  • Default unit runs skip native integration compilation; explicit integration tests pass. A restricted-PATH native build passed with only System32 available before the build script added its tool directories.
  • CLI typecheck, changed-file formatting and whitespace checks passed. Root check:quick and the production CLI build passed with the new per-output hook. Earlier full Electron and unsigned Windows x64 packaging/runtime probes also passed.
  • Previous head a801165 passed both x64 and ARM64 native/adapter CI jobs and the same-run Linux npm archive job, which verified both executable architectures and the launcher. A local dual-architecture npm prepack/archive check passed using the current x64 build plus that CI ARM64 artifact (native C++ source unchanged). At 8a53cfd, both architectures and the same-run Linux package job also passed.
  • No installed application was replaced or real user session terminated. Real-provider memory-baseline recovery remains unproven; exact-head CI also passed.

Context handoff

Instructions for reviewing agents

  • Review focus: Atomic job membership before resume, owner/control-channel lifetime, handle inheritance, launch compatibility, fail-closed assets, and packaged runtime behavior.
  • Decisions to challenge: A dedicated supervisor owns each job; a Node launcher retains cross-spawn resolution; Windows launches require matching packaged assets.
  • Plausible failures / evidence gaps: Exact-head x64/ARM64 native CI and same-run cross-architecture npm packaging passed. Native provider residency and real-provider memory recovery remain unproven.

Authoring context

  • User goal / directives: Implement Windows durable process ownership as the third layer of a native three-PR stack.
  • Constraints / non-goals: Preserve unrelated work; no global process killing, provider internals changes, installed-app replacement, or automatic upstream merge.
  • Risk-bearing decisions: Owner or supervisor death terminates the private job; ownership setup failures stop launch rather than falling back to unowned processes.
  • Destructive or irreversible behavior: Closing the private job terminates its owned descendants. Tests target only synthetic processes with independently captured OS handles.
  • Deliberately not done or tested: ARM64 execution on the local x64 host and live provider memory recovery. The current-head same-run Linux packaging job passed.
  • Unknowns / confidence: Native source and packaged Electron probes passed previously; both architectures passed CI at 615143c. Rebased-head handle/adapter/integration tests passed at 8a53cfd; current CI and same-run package verification also passed.

Latest review verification

At 8a53cfd, the stack includes the eight latest the first PR review resolutions from 720e84c: PTY admission/draining and retry retention, authentication cleanup ownership, startup owner admission, spawn-error handling, explicit Windows termination semantics and non-retryable cleanup failures. the third PR supplies the requested descendant Job Object ownership. Local composed verification and exact-head fork CI Static checks, Tests and Desktop E2E all passed. Upstream CI on these recreated PRs is evaluated separately. The top PR also passed Windows x64, Windows ARM64 and the same-run dual-architecture npm archive verification.

Copilot AI lite review requested due to automatic review settings September 7, 2026 05:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

@slashdevcorpse, this pull request needs updates before review.

It is marked status:needs-pr-attention. Address the findings below by 2026-09-15 08:20:24 UTC. The label and this comment are removed automatically after the PR passes validation.

If the PR remains invalid for 7 days, it will be closed and marked status:pr-policy-expired. Continue afterward by opening a new pull request with the current template.

Policy findings
PR does not meet Lody contribution requirements:

- PR changes 7220 lines; community PRs over 1000 lines require a maintainer assignment on the linked Issue before review.

See `CONTRIBUTING.md` and `.github/PULL_REQUEST_TEMPLATE.md`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants