Skip to content

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

Closed
slashdevcorpse wants to merge 3 commits into
fix/background-session-resource-historyfrom
fix/windows-job-process-ownership
Closed

[3/3] fix: own Windows process trees with native job supervision#4
slashdevcorpse wants to merge 3 commits into
fix/background-session-resource-historyfrom
fix/windows-job-process-ownership

Conversation

@slashdevcorpse

@slashdevcorpse slashdevcorpse commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Related issue

Refs LodyAI#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.

Stack order

Native GitHub stack LodyAI#6 in slashdevcorpse/Lody, created with gh stack link:

  1. [1/3] fix: bound Windows ACP shutdown and retain failed cleanup #2 — bounded shutdown and retained cleanup ownership; base main.
  2. [2/3] fix: protect background sessions and record resource history #3 — background-aware eviction and bounded resource history; base fix/windows-process-tree-cleanup.
  3. [3/3] fix: own Windows process trees with native job supervision #4 — native Windows Job Object ownership; base fix/background-session-resource-history.

Replaces the closed cross-fork LodyAI#430 and LodyAI#435. Each PR has an incremental diff against its predecessor. GitHub native stacks require all branches in the same repository, so this stack is in the contributor fork. It has not merged or landed upstream; LodyAI#429 remains open.

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 PR #2 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. PR #4 supplies the requested descendant Job Object ownership. Local composed verification and exact-head GitHub Static checks, Tests and Desktop E2E all passed. 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 6, 2026 05:19

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.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 66e9e696-2429-4868-83f0-41a620848e0d


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 23 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread apps/cli/src/agent/acp-runner.ts
Comment thread apps/cli/scripts/build-windows-process-supervisor.mjs
Comment thread apps/cli/vite.config.ts Outdated
Comment thread apps/cli/src/utils/windows-owned-process.integration.test.ts Outdated
Comment thread apps/cli/src/utils/windows-owned-process.integration.test.ts
Comment thread .github/workflows/ci.yml
@slashdevcorpse
slashdevcorpse force-pushed the fix/windows-job-process-ownership branch from 615143c to a801165 Compare September 6, 2026 06:14
@slashdevcorpse
slashdevcorpse force-pushed the fix/windows-job-process-ownership branch from a801165 to 8a53cfd Compare September 6, 2026 06:44
@slashdevcorpse

Copy link
Copy Markdown
Owner Author

Superseded by upstream PR LodyAI#458. The series now lives in LodyAI/Lody as LodyAI#456 -> LodyAI#457 -> LodyAI#458. Source branches and review history are preserved.

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