Skip to content

fix(desktop): bind backend teardown to retained process authority (#89614) - #90250

Closed
andrexibiza wants to merge 1 commit into
NousResearch:mainfrom
andrexibiza:fix/89614-process-authority-fail-closed
Closed

andrexibiza wants to merge 1 commit into
NousResearch:mainfrom
andrexibiza:fix/89614-process-authority-fail-closed

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Scope

Implements the Desktop-owned process-authority slice of #89614 across Windows and POSIX.

A PID is observation, not destructive authority. Teardown may mutate only the retained object whose identity and generation were proved at acquisition.

The broader repository-wide destructive-process-control class remains owned by #89614.

Authority model

Windows

Each marked Desktop backend starts inside a generation-bound Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. Bootstrap verifies the Electron parent through a retained kernel handle and creation-time marker, assigns the backend through GetCurrentProcess(), and retains both handles. Malformed envelopes, recycled-parent identity, handle failures, and assignment failures abort instead of falling back to taskkill or reconstructed PID authority.

POSIX

A retained supervisor owns the Desktop root session. Contained start_new_session=True children receive retained nested owners: the owner remains in the caller group, proves the target’s private SID/PGID after exec, routes local termination through retained authority, reaps on parent loss, and composes recursively. Root teardown drains nested scopes before final force. Direct setsid, setpgid, and setpgrp remain rejected where no retained owner can prove the resulting scope.

This closes the earlier collision where flattening private-session children into the Desktop root widened MCP/LSP-local cleanup into backend-wide teardown.

Terminal generation barrier

A force request is not terminal proof. If a retained backend owner has received the platform force command but has not emitted its exact exit event, the generation barrier blocks startAttempt() from minting a replacement backend. The barrier clears only when that retained owner supplies terminal exit proof. This prevents overlapping old/new backend generations after a local teardown wait times out.

Exact green object

The active PR now contains one commit only. Every visible commit therefore has one terminal green verdict; no superseded red intermediate object remains in the commit list.

Witnesses cover retained Windows authority, POSIX root and nested ownership, local subtree teardown without collateral root loss, root teardown of nested residue, MCP watchdog isolation, asyncio/LSP private-session behavior, generation fencing, and the terminal generation barrier at the real backend connection-state boundary.

Provenance-preserving history repair

The original development chain is preserved by exact SHA for authorship, chronology, and review provenance, but is no longer active Git ancestry. Exact-SHA certification proved that at least the first historical object contained a deterministic Desktop lint defect; an immutable commit cannot be made green by rerunning it. The final tree was therefore republished as one corrected current-main object rather than hiding red checks behind a newer head.

Superseded 21-object development lineage

8f60d098d2c21e72f86868c44697c83a94f546c9
2156712bb7b615717e1d8a64754c6db6158ae033
2f605accd80ef59509781ffc7e75da034b84c606
89e22c88eab98d586a221b30f94050eeb0545e75
234c8948a15e3335b5595a85cfd20b0a469faaca
744aae1fd48e3a9368e6a508d9d2560396483357
409d59128e1f7f34fed1b1ecc7e09aec3b09bbdf
908af32bc766a592fefe346b491dd4d78adaf776
af52aa979981c3df5c45e6a2b6017eaf58af69da
3a24f26ab31597b8aafa7fdb2e22f9fd656a6a8d
23710e9683d0c4b1931cceabc3227eaec5145cc5
842e3498b8053684d687c6c16e7c0e5264d8797e
0aad35adedb0fd527617ab8e11c4fd38c7c4daf6
f79f93443db305579ba70ac03126d91eec65e31a
6226c86f01b31f00bddf6b6213e21684e5cea62d
eacad329f3938fe1302066de95eac8c1cb8015c0
695d2563c1b2bdd6edb9f2d7a11cf1d6e0ad8813
ba12d40adf2481a4e649fcf58db89e1288c42b69
d68ceb331d2418471ea002ceb7ab7785e27410cf
305ade0f49fa4914ceea0924396b91ce41a44a1d
2eb863923e31e5eefebc2f62a4d421a93030dce8

The temporary exact-object certification lane #91709 is closed unmerged after establishing that reruns cannot repair immutable source defects. No implementation or attribution was discarded.

Provenance and boundary

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage area/install-update Installer, updater, packaging, wheels, doctor P1 High — major feature broken, no workaround sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 19, 2026
@andrexibiza
andrexibiza force-pushed the fix/89614-process-authority-fail-closed branch from 31696ec to 2156712 Compare August 19, 2026 21:02
@andrexibiza andrexibiza changed the title fix(desktop): fail-closed process authority — eliminate PID-based tree-kill (#89614) fix(desktop): bind backend teardown to Windows Job Object authority (#89614) Aug 19, 2026
@andrexibiza
andrexibiza marked this pull request as ready for review August 19, 2026 21:03
@spfcraze

Copy link
Copy Markdown
Contributor

Verified peer review — detailed pass. The Windows Job Object design is the right fix for #89614, and I want to be clear up front that the core authority mechanism is strong. The verified concerns below are scoped to what I could confirm against the code and tests; the primary one is a cross-platform regression I think is worth resolving before merge.

== PRIMARY: POSIX process-group teardown was removed with no POSIX replacement (tree/orphan regression) ==

What the old code did (and why):

  • The pre-change backend-child.ts explicitly handled the POSIX case by signalling the backend's ENTIRE process group: process.kill(-pid, 'SIGTERM'), escalating to process.kill(-pid, 'SIGKILL'), with a direct-child fallback. Its own module docstring cites the reason verbatim: "On POSIX the backend IS spawned into its own session/process-group (start_new_session=True), so child.kill('SIGTERM') would only reach the backend and orphan its MCP grandchildren (the leak in #serve-orphans). We signal the whole group via process.kill(-pid, ...) instead."
  • So on POSIX, teardown reached the backend AND its gateway/MCP/pty grandchildren in the same process group.

What the new code does:

  • backend-child.ts now only signals the retained direct ChildProcess: stopBackendChild -> child.kill('SIGTERM'); forceStopBackendChild -> child.kill('SIGKILL'). The entire process-group branch (killGroup / negative-pid send) is gone.
  • The Windows Job Object (KILL_ON_JOB_CLOSE) that owns tree-wide teardown exists ONLY on Windows. There is no POSIX equivalent in this change.

Verified impact:

  • Every teardown path now direct-kills only: stopOwnedBackend, stopPoolBackend (via pool-stop.ts), waitForBackendExit, stopBackendTreesForUpdate, and the failed-process path all route through the same direct-child-only signal.
  • hermes_cli/gateway.py spawns backend grandchildren with start_new_session=True (its own process groups), so a direct SIGTERM to the backend Python does not reach them.
  • pool-stop.ts still carries a comment saying "Signal the child (tree/group kill per platform)" but the underlying stopChild implementation on this head is direct-child-only on every platform — that comment is now stale and could mislead future maintainers.

Why it matters: on POSIX this regresses the exact #serve-orphans leak the prior code was built to prevent. When the desktop quits or a profile backend is evicted, the backend's gateway/MCP grandchildren can survive with the venv shim still locked, and the subsequent isShimLocked wait may loop until timeout or leave residue.

Evidence: the PR's own new tests (backend-stale-pid.test.ts, backend-process-authority-env.test.ts) only assert that a retained direct child receives 'SIGTERM'/'SIGKILL'. There is no POSIX orphan / grandchild / process-group test on this head. I ran the Python authority + bootstrap suites (14/14 pass) but they exercise the Windows Job Object on a mocked API and do not cover the POSIX Electron-side group teardown.

Suggested options (appreciate your call):

  • Option A (recommended): restore a retained-authority group signal on POSIX only. Since the retained ChildProcess is verified live (isLiveProcessRoot), signal its process group via the group leader captured at spawn (or process.kill(-pid, signal) with a direct-child fallback) on non-Windows. This preserves the design's core principle — never rebuild authority from a bare PID; the negative-pgid send is still derived from the retained pid of a child we already hold and confirmed live — while closing the POSIX orphan gap the old code handled.
  • Option B: keep direct-child-only on POSIX, but explicitly document that POSIX teardown now leaves gateway/MCP grandchildren outside the backend to be handled by the OS/sibling cleanup, and add a POSIX test asserting the group is NOT signalled so the behavior change is at least intentional and pinned.
    Either way I'd suggest one POSIX test that spawns/emulates a child with a moved process group and asserts teardown does (A) or does not (B) reach it, matching the chosen contract.

== MINOR 1: parent_started_at_ms estimation relies on clock consistency ==

Electron sets HERMES_DESKTOP_PARENT_STARTED_AT_MS = Math.round(Date.now() - process.uptime() * 1000), computed at backend-spawn time, and Python compares it to GetProcessTimes(parent) with a 5s tolerance. This is normally accurate to well under 5s, and the tolerance absorbs ordinary jitter, so I don't consider it a bug. But it is a clock-consistency reliance: an NTP/clock jump after Electron started (or a very long-lived Electron where process.uptime() diverges from kernel creation time) could cause a false parent-generation-mismatch that aborts a marked backend and, because setup is fail-closed, blocks that backend launch until relaunch. Suggested fix: instead of estimating, read the parent's real creation time from the OS at spawn and pass that exact value (e.g., via GetProcessTimes-equivalent on the Electron side, or snapshot the marker once at Electron boot rather than re-deriving at each spawn). Passing a single authoritative marker captured once at Electron startup would also make it stable across a clock change.

== MINOR 2: stopOwnedBackend now throws NO_PROCESS_AUTHORITY on POSIX with no recovery path ==

The pre-change code, when a backend would not stop, revalidated identity then SIGKILL'd — with PID-reuse protection. The new code deliberately refuses to reconstruct authority from a PID (throw NO_PROCESS_AUTHORITY), which is the right security posture for Windows. But on POSIX there is now no Job Object and no fallback, so a stuck/lingering backend can no longer be force-stopped at all; the shim-lock wait in releaseBackendLock can loop until its 15s timeout and leave residue. I'd suggest confirming this is acceptable on POSIX (it may be, given the design intent to never kill by reconstructed PID), and if so, documenting it as an intended cross-platform behavior difference in the PR body / code comment so maintainers don't rediscover it. If a bounded, authority-confirmed group signal (Option A above) is adopted, it would also give POSIX a graceful recovery that avoids PID-reconstruction.

== Overall ==
No blocker on the Windows Job Object authority itself — the retained-handle creation-time verification, fail-closed arming, and handle-cleanup are exactly right, and the 14 Python authority/bootstrap tests pass. The POSIX process-group regression is the one I'd want decisions (Option A or B) and a POSIX test on before merge; the two minor items are robustness/documentation nits. Happy to help test a proposed POSIX solution or a suggested-fix iteration.

andrexibiza commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor Author

Superseded by the authoritative final status after history repair. The active PR now contains one exact green landing commit; the earlier development SHAs remain preserved as provenance in the PR body.

@jackulau

Copy link
Copy Markdown
Contributor

Reviewing this as the author of the superseded slice (#89689). I agree with the topology and I am not defending my head - the Job Object is the right authority and I said as much when I declined to reach for a cheaper predicate. Three things from reading this one, one of which is currently red.

1. The typecheck failure is in a file this PR does not touch, which is the interesting part

JS & TS checks / apps/desktop / check:lint is failing on:

electron/backend-env.test.ts(123,9): error TS2339: Property 'PATH' does not exist on type
  '{ PYTHONPATH: string; PYTHONUTF8: any; HERMES_DESKTOP_PARENT_PID: string;
     HERMES_DESKTOP_PARENT_STARTED_AT_MS: string; HERMES_DESKTOP_PROCESS_AUTHORITY: string;
     HERMES_DESKTOP_PROCESS_GENERATION: string; } | { ...; }'

backend-env.test.ts is not in this PR's diff. So this is not a test that needs updating - it is the inferred return type of buildDesktopBackendEnv changing shape underneath an untouched caller.

Read the union in the error: both members carry PYTHONPATH and PYTHONUTF8, one carries the four HERMES_DESKTOP_* keys, and neither carries PATH or Path. The computed property is still there in the source and is unchanged by this diff:

[key]: buildDesktopBackendPath({ ... })

What changed is that the return object now begins with a conditional spread whose operand is itself a union:

const processAuthority = platform === 'win32' ? { HERMES_DESKTOP_...: ... } : {}
return { ...processAuthority, PYTHONPATH: ..., PYTHONUTF8: ..., [key]: ... }

The spread splits the return type into two branches, and the computed key drops out of both. The PATH entry is still produced at runtime; it has just stopped being expressible in the inferred type, so every existing .PATH / .Path read in the old test is now an error.

Patching the test would hide this rather than fix it. The function's honest contract is a string map that callers merge into process.env, so annotate it and the inference problem disappears at the source:

function buildDesktopBackendEnv({ ... }: any = {}): Record<string, string> {

That also keeps the next conditional key from silently re-erasing something. Worth doing before the head is verified, since All required checks pass is gated on it.

2. The 5-second tolerance is the one place a number is still doing work

_PARENT_START_TOLERANCE_MS = 5_000
...
if abs(observed_start - spec.parent_started_at_ms) > parent_start_tolerance_ms:

I want to be clear this is not a "remove the tolerance" comment - the tolerance is necessary given the marker you are passing, and that is exactly my point. The Electron side sends:

parentStartedAtMs = Math.round(Date.now() - process.uptime() * 1000)

which is an estimate of the parent's start, derived from V8 runtime uptime, while process_started_at_ms on the Python side reads the kernel's creation time via GetProcessTimes. Those two clocks do not start at the same instant - the kernel creates the process before the runtime begins counting - so the tolerance is absorbing that skew, not real jitter. 5s is a generous but reasonable cover for it, and if it were tighter this would fail closed on slow cold starts, which for this design means aborting the backend.

The cost of paying for that skew with a window: within it, the check degrades to "a process with the right PID that started at roughly the right time", which is the same shape of reasoning the invariant is meant to retire. PID reuse inside 5 seconds and landing inside the window is remote, and I am not claiming it is reachable in the field. But this PR is the one that gets to say "a PID is observation, not authority", so it is worth being explicit that a 5s equivalence class is the residue and that its width is set by a measurement choice, not by a threat model.

If you want the window closed rather than justified, the skew is removable: have the parent's marker be measured rather than estimated so the comparison can be exact. Electron cannot call GetProcessTimes on itself without native code, which I assume is exactly why the authority lives in Python - so the honest options are (a) keep the tolerance and document that it covers runtime-vs-kernel skew rather than clock drift, with the constant named for what it absorbs, or (b) have the bootstrap read both its own parent handle's creation time and a value the parent obtained the same way. (a) is fine by me; I mostly want the comment on the constant to say which of the two it is, because a future reader tightening it to 500ms would brick cold starts and never learn why.

Related: parentStartedAtMs is computed as a default parameter of buildDesktopBackendEnv, so it is evaluated per call rather than per app launch. That is correct today because Date.now() - uptime*1000 is stable across calls, but it is stable by arithmetic accident rather than by construction - if it ever becomes a real measurement it should be captured once at app start and passed down, or two backends spawned seconds apart can disagree about when their common parent started.

3. On the POSIX half

I agree with the peer review that direct-child-only signalling is a regression on POSIX, and I want to be precise about which half of my PR it comes from, since I wrote it: removing process.kill(-child.pid, 'SIGTERM') was the correct removal of a pid-based mutator, but it deleted descendant coverage without replacing it, and I priced that in my body as accepted residue for a Windows bugcheck. That trade reads differently once Windows has real authority and POSIX has none. Whatever lands should not restore kill(-pid); the POSIX analogue of the Job Object is a process group or session the parent holds, and setting detached: true at spawn so the child is a group leader is what makes that authority real rather than reconstructed.

Disposition

Agreed on all four points in your comment on #89689. I will leave it open as the root-cause record, will not push for independent merge, and will close it as superseded once this head is verified. Thanks for keeping the provenance explicit - that is more than I expected and more than was required.

Happy to send the return-type annotation from item 1 as a patch against this branch if that is faster for you than folding it in.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed exact head 89e22c88eab98d586a221b30f94050eeb0545e75 against recorded base 657550716f370bd5d1e848a57fc24b9c404cf982 and current main 258410a184e507485ec7eb0c366ad1ce64a328a7. This head is 4 commits ahead / 104 behind current main. Docker and Nix are green; hosted CI is red only in the new POSIX authority suite. There were no prior formal review submissions on this head, so this is not duplicating an unchanged-head review.

The Windows half is materially stronger than the original containment-only shape: stale/PID-only records are non-actionable, the Desktop launch carries a generation envelope, bootstrap occurs before hermes_cli.main, and the Job Object retains kernel authority over the contained generation rather than reconstructing a process tree from a PID. The new POSIX supervisor is also the right direction: Electron retains the supervisor handle, the backend becomes a session leader, and natural/forced teardown can address the owned group without a later tree walk.

I still see four blockers before this satisfies the merge gate already written into the PR body.

1. transferred is currently a declaration, not an authority handoff

desktop_child_env(lifetime="transferred"|"foreign", transfer_receipt=...) accepts any non-empty string as sufficient proof that another owner accepted the process. _normalize_child_env() then strips the Desktop authority envelope and allows setsid/detachment. The test demonstrates the problem directly with the literal receipt test:external-owner-accepted: there is no receiving authority, retained OS object, generation binding, or acknowledgement from a new owner before the old owner releases scope.

That is weaker than this PR's own stated requirement: “every deliberate setsid, detached spawn, or equivalent escape [must] complete an authority handoff before the old owner may release it.” A caller can currently self-authorize escape by inventing a string.

The exact-head CI failure is the same architectural gap showing up in the harness. test_receipted_transfer_survives_old_owner_and_is_explicitly_cleaned_up creates an escaped child with a ceremonial receipt, then has no legitimate receiving owner to clean it up, so it falls back to raw os.killpg() and the repository live-system guard correctly refuses that destructive mutation outside the test authority.

Please make transfer a real protocol, not metadata: the receiving owner should establish/retain its execution-scope authority first, issue a receipt bound to the exact child/generation/incarnation, and own terminal cleanup. The acceptance test should terminate the transferred child through that receiving authority, not through reconstructed PGID mutation. This also gives us the missing transfer-after-crash and generation-fence witness.

2. Existing intentional POSIX detach consumers are not migrated, so the global guard silently changes their lifecycle

The POSIX authority monkeypatch intercepts every descendant subprocess.Popen and forces start_new_session=False for the default contained lifetime. But production code already has deliberate detach semantics whose correctness depends on leaving the caller's session.

The clearest case is hermes_cli/gateway.py: its update/restart watcher explicitly uses start_new_session=True so the watcher and respawned gateway survive the CLI/gateway process exiting; the comments call this out as required behavior. There is no production callsite using desktop_child_env(..., lifetime="transferred", ...) to hand that process to another owner. A repository search finds the new helper only in the authority module/tests, not in these existing lifecycle consumers.

So under a POSIX Desktop backend, this patch can silently turn an intentionally independent restart watcher back into a contained child, then reap it when the old Desktop authority closes. That is the opposite side of the containment problem: preventing accidental escape also prevents intentional survival unless every legitimate escape is migrated.

Please audit the intentional detach/session callsites (gateway restart/update is the minimum required witness), classify them as contained/transferred/foreign, wire them through the real transfer protocol from blocker 1, and add a Desktop-POSIX end-to-end restart/update test proving the watcher survives the old owner and is owned by the new one. This should include current-main gateway changes during the rebase, not only the old recorded base.

3. Contained descendants can still escape via nonzero process-group requests

The guard neutralizes start_new_session=True, process_group == 0, setsid=True, and setpgroup == 0. It does not reject or rewrite a positive nonzero process_group passed to Popen, nor a positive nonzero setpgroup passed to posix_spawn/posix_spawnp.

That leaves a hole in the stated invariant “unreceipted descendant cannot escape retained scope.” A contained child that selects another permitted PGID can leave the supervisor's kill group without any transfer receipt at all. The current topology tests cover start_new_session=True, but not the positive-group form.

For contained, every session/process-group mutation request should either be rejected or normalized back into the retained scope; only a completed transfer may authorize a different group/session. Add real regressions for positive process_group and setpgroup values, not just the zero/new-group spelling.

4. Typed terminal outcomes exist in backend-child.ts, but production lifecycle callers still collapse or discard them

The PR body correctly makes typed terminal outcomes part of the merge gate. The implementation has the vocabulary (Exited, AlreadyExited, NoAuthority, PermissionDenied, TimedOut) and stopBackendChildAndWait(), but the production call graph still routes through compatibility booleans/void:

  • stopBackendChild() / forceStopBackendChild() collapse the structured result to boolean;
  • main.ts::stopBackendChild() ignores the compatibility result;
  • waitForBackendExit() returns void; after the graceful wait it calls the boolean force wrapper and, after one more second, returns without surfacing a terminal timeout;
  • releaseBackendLock() calls stopBackendTreesForUpdate() and waitForBackendExit() without consuming a terminal result;
  • signalRetainedChild() names a successful signal submission Exited before an exit has actually been observed.

That means updater/pool/profile/reconnect callers still cannot distinguish “signal accepted”, “exited”, “permission denied”, and “timed out” at the mutation boundary. The new type exists, but the authority decision is not yet propagated to the consumers that must fail closed on ambiguity.

Please make the retained-owner stop API terminally truthful: signal acceptance is not Exited; lifecycle callers should await/consume stopBackendChildAndWait() (or equivalent) and explicitly preserve residue/abort on every non-terminal result. Remove or confine the boolean compatibility path so production teardown cannot silently fall back to it.

CI and current-main state

Exact-head hosted CI currently has one failure: tests/hermes_cli/test_posix_process_authority.py::test_receipted_transfer_survives_old_owner_and_is_explicitly_cleaned_up; 2,826 tests in that slice pass and the failure is the live-system guard blocking the test's raw killpg cleanup of the escaped child. Docker and Nix both pass. I would not paper over that guard: replace the fake transfer with a receiving authority and let the test prove the ownership transfer end-to-end.

The branch is now 104 commits behind current main and apps/desktop/electron/main.ts has moved upstream, so a rebase + fresh exact-head matrix is required after the authority changes.

Topology / credit

  • #89614 / @gebilaowang404 remains the concrete P1 incident owner. This PR correctly remains Refs, not Fixes; it addresses the Desktop authority slice, not every destructive-process mutator in the repository.
  • #90144 / @andrexibiza is the architectural class owner: proof scope must equal mutation scope. The retained Job/session direction is aligned with it; the ceremonial transfer receipt is not yet a scoped mutation permit.
  • #89689 / @jackulau is superseded, not duplicate. Its key contribution was identifying the update-loop stale-handle/recycled-PID mechanism and removing PID-derived mutation; that provenance should remain explicit as this branch replaces containment-only teardown with retained authority.
  • Existing service-manager and detached-gateway paths are adjacent owners, not descendants to absorb accidentally. Their lifecycle authority must be composed explicitly rather than globally neutralized by the subprocess guard.

Re-review gate: real receiving-owner transfer protocol; migration of intentional detach consumers with gateway restart/update witness; closure of the positive process-group escape; terminal typed outcomes propagated through Desktop lifecycle callers; rebase on current main; fresh Linux/macOS/Windows + CI/Docker/Nix evidence.

@andrexibiza andrexibiza changed the title fix(desktop): bind backend teardown to Windows Job Object authority (#89614) fix(desktop): bind backend teardown to retained process authority (#89614) Aug 20, 2026
@alt-glitch alt-glitch removed the area/install-update Installer, updater, packaging, wheels, doctor label Aug 20, 2026
@alt-glitch alt-glitch added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state comp/cli CLI entry point, hermes_cli/, setup wizard labels Aug 20, 2026

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-reviewed the materially changed exact head af52aa979981c3df5c45e6a2b6017eaf58af69da, not the old 89e22c88eab98d586a221b30f94050eeb0545e75 review object. Current main is 4a5b6dd4512a10c3c18da3e5b9e5c7fb681cbfbb; GitHub built the exact synthetic merge 2e96f1bcbe89d9498b50a9d76221725ff07d6ff6 with those two parents. Exact-head CI 32414968475, Nix 32414967625, and Docker 32414967613 are all green.

The four blockers from my previous review are materially repaired: transfer is now a one-shot ProcessTransferGrant with child-side ACK rather than a caller string; windows_detach_popen_kwargs() is adapted into a receiving-owner handoff; positive process_group / setpgroup escape paths are folded back or rejected; and Desktop stop semantics now distinguish StopRequested, observed Exited, AlreadyExited, NoAuthority, and hard BackendStopError outcomes. I would not carry those old findings forward.

I do see one new architecture blocker from composing the now-global POSIX descendant guard with existing current-main process owners.

Blocker — contained currently collapses legitimate nested process authority into the Desktop root group

hermes_cli/_posix_process_guard.py::guarded_popen_init() treats every ordinary descendant as contained and rewrites:

  • start_new_session=True → False;
  • any process_group → None;
  • posix_spawn(..., setsid=True/setpgroup=...) back into the retained root scope.

That correctly prevents an unreceipted descendant from escaping the Desktop owner. But it conflates two different shapes:

  1. ownership escape — a child must survive / leave the Desktop owner (transferred / foreign); and
  2. nested owned scope — a child remains owned by Desktop, but an existing subsystem deliberately creates a narrower process group so its own destructive control cannot widen onto siblings or the parent.

Current main has production code in category (2), and the guard currently erases that authority boundary.

Concrete collision: tools/mcp_stdio_watchdog.py

The watchdog deliberately starts the real MCP command with start_new_session=True. Its module contract says why: the real command gets its own process group so _terminate_process_group() can reap that MCP tree without touching the watchdog/parent group. Shutdown then does:

pgid = os.getpgid(proc.pid)
killpg(pgid, sig)

Under this PR's Desktop POSIX bootstrap, PYTHONPATH carries desktop_bootstrap/sitecustomize.py into Python descendants and normalize_child_env() stamps _HERMES_DESKTOP_POSIX_DESCENDANT_GUARD. So when the Python watchdog starts the real MCP child, the descendant guard is active and rewrites that start_new_session=True to False.

The consequence is not merely “less isolation”: os.getpgid(proc.pid) now returns the inherited Desktop/backend process group. _terminate_process_group() can therefore send SIGTERM/SIGKILL to the entire retained Desktop group when it believes it is terminating one MCP subtree. The local proof (“this proc is my MCP child”) has been widened by the guard into authority over unrelated contained siblings and the backend itself. That is exactly the #90144 defect class, just in the inverse direction from the stale-PID bug.

Same missing axis: agent/lsp/client.py

The LSP client also deliberately requests start_new_session=True, with an explicit current-main comment that inheriting the gateway/TUI PGID lets MCP orphan cleanup capture the LSP PID and then killpg() the parent group. The descendant guard removes precisely the separation that code added to prevent that cross-owner kill.

So the current topology tests prove “grandchildren cannot escape the root PGID”, but they do not prove “existing child controllers retain a mutation scope no broader than the child subtree they own.” Green exact-current-main CI does not cover this composition.

Required repair

Please do not solve this by simply allowlisting those start_new_session=True calls: an unregistered subgroup would then escape the outer supervisor's root-PGID reap. The model needs a third POSIX authority state between same-PGID contained and ownership-releasing transferred/foreign: a nested owned scope (or equivalent registered child-scope permit) whose subgroup is retained by the Desktop generation, can be signalled by its immediate owner, and is still reaped by the outer authority on root teardown.

At minimum:

  • audit production start_new_session=True / explicit process-group users under a Desktop backend and classify them as same-scope contained, nested-owned, transferred, or foreign; windows_detach_popen_kwargs() only covers intentional survival/detach and is not sufficient for nested child-control scopes;
  • migrate tools/mcp_stdio_watchdog.py and agent/lsp/client.py to the nested authority path rather than flattening their group identity;
  • add a real descendant-guard topology witness: start the MCP watchdog under the Desktop POSIX authority, start its real child, trigger watchdog cleanup, and prove the MCP subtree dies while the backend + unrelated contained control process remain alive; then tear down the Desktop owner and prove any registered nested scope is also reaped;
  • add the complementary LSP witness that under the guard its child-control scope cannot resolve to the backend/TUI PGID. Run these on Linux and macOS.

There are several other direct start_new_session=True users on current main (process registry/systemd scope, verify runner, host supervisor, local execution, etc.). I am not asserting each is broken; they need classification because the global guard currently changes all of them.

Topology / provenance

  • #89614 / @gebilaowang404 remains the concrete P1 incident owner. This PR should continue to Refs, not Fixes, because repository-wide destructive-process closure remains larger than Desktop.
  • #90144 is directly implicated: proof scope and mutation scope have to remain equal inside retained ownership too; flattening nested authority can widen a child-local kill onto the root group.
  • #89689 / @jackulau remains superseded, not duplicate. Its recycled-PID/update-loop diagnosis is still provenance for why PID-derived mutation had to go.
  • MCP watchdog and LSP lifecycle are adjacent existing owners, not escapees to neutralize. Their narrower groups are part of their safety contracts and need composition with the new Desktop authority.

One non-code cleanup after the repair: the PR body still names 234c8948... as the current head / one-commit final repair even though the live head is af52aa97...; please refresh the exact implementation/receipt section so review metadata matches the object being landed.

Re-review gate: nested-owned process authority composed with the existing MCP/LSP group-control contracts, current-main audit of direct group/session creators, real Linux/macOS topology witnesses, then fresh exact-head CI/Nix/Docker.

@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth and removed sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P1 High — major feature broken, no workaround comp/cli CLI entry point, hermes_cli/, setup wizard labels Aug 21, 2026
@andrexibiza
andrexibiza force-pushed the fix/89614-process-authority-fail-closed branch from 2736e54 to 13206fc Compare August 21, 2026 17:34
@andrexibiza
andrexibiza marked this pull request as draft August 21, 2026 17:34
@andrexibiza
andrexibiza force-pushed the fix/89614-process-authority-fail-closed branch from 13206fc to f79f934 Compare August 21, 2026 17:34
@andrexibiza
andrexibiza marked this pull request as ready for review August 21, 2026 17:36
@andrexibiza
andrexibiza force-pushed the fix/89614-process-authority-fail-closed branch from f79f934 to e07835f Compare August 21, 2026 17:37
@andrexibiza
andrexibiza marked this pull request as draft August 21, 2026 17:37
@andrexibiza
andrexibiza force-pushed the fix/89614-process-authority-fail-closed branch from e07835f to f79f934 Compare August 21, 2026 17:38
@andrexibiza
andrexibiza marked this pull request as ready for review August 21, 2026 17:42
andrexibiza added a commit to andrexibiza/hermes-agent that referenced this pull request Aug 21, 2026
Clean one-commit landing for the verified Windows Job Object and POSIX retained nested-owner architecture from NousResearch#90250. Preserve explicit transfer, typed stop outcomes, MCP/LSP subtree isolation, generation fencing, and the full topology proof surface without carrying superseded intermediate CI objects.

Refs NousResearch#89614, NousResearch#90144, and NousResearch#90250. Supersedes NousResearch#89689.
@andrexibiza andrexibiza reopened this Aug 21, 2026
@andrexibiza
andrexibiza marked this pull request as draft August 21, 2026 17:43
@andrexibiza
andrexibiza marked this pull request as ready for review August 21, 2026 17:44
@andrexibiza
andrexibiza marked this pull request as draft August 21, 2026 17:48
@andrexibiza
andrexibiza marked this pull request as ready for review August 21, 2026 17:48
Land the complete Desktop Windows Job Object, POSIX retained nested-owner, typed stop-outcome, MCP/LSP isolation, generation-fencing, and terminal generation-barrier architecture as one current-main commit.

The superseded 21-commit development lineage remains preserved by exact SHA in the pull-request provenance record, but is removed from the active commit list because several immutable intermediate objects are deterministically red and cannot be made green without replacement.

Refs NousResearch#89614, NousResearch#90144, and NousResearch#90250. Supersedes NousResearch#89689.
@andrexibiza
andrexibiza force-pushed the fix/89614-process-authority-fail-closed branch from 2eb8639 to f6b09db Compare August 21, 2026 18:04

Copy link
Copy Markdown
Contributor Author

Final status after history repair

This is the authoritative current-state receipt for #90250 after the branch and CI cleanup.

Exact-object verification

What changed in the history

The earlier 21-object development chain contained intermediate commits with deterministic source defects and red/cancelled checks. Rerunning an immutable SHA cannot repair its source. The complete corrected final tree was therefore republished as the single current-main commit above, so the active PR no longer presents intrinsically red intermediate objects as landing commits.

The original 21 SHAs, authorship, chronology, review submissions, architecture decisions, and supersession links remain preserved in the PR body and the canonical Notion record. They are provenance, not active landing ancestry. No implementation scope or attribution was discarded.

Architecture now represented by the landing object

  • generation-bound retained Windows Job Object authority;
  • retained POSIX root and nested process-scope ownership;
  • explicit transfer and typed stop outcomes;
  • MCP/LSP-local teardown that cannot widen onto the backend group;
  • root teardown of nested residue;
  • a terminal generation barrier that blocks replacement startup until the exact prior owner emits exit proof.

No source, exact-head CI, base-alignment, thread, or mergeability blocker remains on this object. The remaining repository transition is upstream acceptance and merge.

@teknium1

Copy link
Copy Markdown
Collaborator

Closing with credit after re-verifying the remaining delta against current main (2026-08-31 desktop salvage sweep, wave 2) — the bug class this PR targets is now covered by merged work:

Parent issue resolved. #89614 (stale-PID taskkill killing recycled PIDs → svchost BSODs) was closed COMPLETED on 2026-08-31 via merged #99558, which installs a fail-closed process-identity guard before every unverified taskkill across the CLI/gateway/update/browser/whatsapp paths.

Desktop-side teardown authority already exists on main. apps/desktop/electron/main.ts gates persisted-PID kills behind identity proof: processIdentityMatches / backendIdentityMatches verify PID + creation-time start marker + backend command line before stopOwnedBackend ever signals, and backendParentMatches does the same for the recorded parent Electron. Live backends are signalled through the retained ChildProcess object (stopBackendChild), not reconstructed PID authority. So the "PID is observation, not authority" invariant this PR argues for is enforced on main for the dangerous (persisted/stale) cases.

What remains in this PR beyond that is a 25-file architecture change — Windows Job Objects with KILL_ON_JOB_CLOSE, a POSIX session-supervisor hierarchy with nested retained owners, a sitecustomize bootstrap shim, and a generation barrier. That is a redesign of the process-lifecycle substrate rather than a fix for a live defect, and peer review on this thread (@spfcraze) identified a concrete regression inside it: the POSIX process-group teardown (process.kill(-pid, …), which exists precisely to reap MCP grandchildren of start_new_session=True backends) was removed without a POSIX replacement. Per the contribution rubric (AGENTS.md: no speculative infrastructure; smallest footprint at the core), we're not taking the substrate redesign now that the motivating bug class is closed.

Credit where due: your generation-barrier observation (a force request is not terminal proof; don't mint a replacement backend until exit is observed) is a genuinely good invariant, and the Job-Object approach is the right Windows primitive — if a live overlapping-generation or job-escape defect shows up on main, a focused issue proposing exactly that slice would be welcome. Also crediting @jackulau's #89689, the earlier retained-handle slice this PR superseded.

Thanks for the substantial engineering effort here, @andrexibiza.

@teknium1 teknium1 closed this Aug 31, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor Author

Thank you 🙏 @teknium1

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) P1 High — major feature broken, no workaround platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants