[AI-1390] Hosted-agent native OS containment (Job Object, PDEATHSIG shim, macOS incarnation identity)#330
Open
realtonyyoung wants to merge 27 commits into
Open
[AI-1390] Hosted-agent native OS containment (Job Object, PDEATHSIG shim, macOS incarnation identity)#330realtonyyoung wants to merge 27 commits into
realtonyyoung wants to merge 27 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Windows - ConPtyProcess.Spawn now creates a Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and binds it to the child at process creation time via a second PROC_THREAD_ATTRIBUTE_JOB_LIST attribute (grown from 1 to 2 entries alongside the existing pseudoconsole attribute), so there is no suspended-then-assign window where the child could exist uncontained. - ConPtyProcess now owns the job SafeHandle; disposing the process closes the job's last handle, which the OS uses to kill the leader and every descendant. - Any failure after CreateProcessW succeeds (but before the ConPtyProcess is fully constructed) tears the child down via the job and confirms death before rethrowing, so callers never observe an ambiguous 'maybe spawned' state. - New ConPtyJobObjectTests (4 cases: dispose kills descendants, breakaway is denied, nesting inside an outer job still works, job-creation failure fails the spawn closed) — Windows-only behavior, so each test no-ops on non-Windows hosts; real coverage is windows-latest in CI. - ConPtyJobObjectTestHelper backs the tests with raw Win32 calls (plain DllImport, since the test project isn't AOT-published and doesn't need AllowUnsafeBlocks for that).
…job handle on CreateProcessW failure Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d/pty_plan_free Adds the parent-side, pre-fork execution-plan construction: PATH/shebang resolution (direct and /usr/bin/env forms, at most one env.PATH resolution hop), an fd-bound execveat/security.capability privilege preflight, and the opaque pty_exec_plan lifecycle (build/contained/free). Linux-only kernel behaviors (execveat, fgetxattr(security.capability)) are #ifdef __linux__ guarded so the same portable PATH/shebang logic still compiles and runs on other POSIX platforms, always degrading to an uncontained plan there (never a false proof of non-privilege). Also fixes two off-by-one argv-allocation bugs found via a standalone harness run under ASan/leaks against the initial transcription (the direct-shebang and env-shebang rewritten argv arrays were both allocated one pointer short of the NULL terminator, corrupting adjacent heap memory once an optional shebang arg or an extra orig_argv element was present). Tests + DummyProcess fixture helpers (shebang scripts, setuid/execute-only copies, dual-PATH dirs) are Linux-gated per spec; Capacitor.Cli.Tests.Unit now needs AllowUnsafeBlocks for the fixtures' own chmod LibraryImport.
…ncontained), NULL-check shebang allocs, guard empty argv Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pture Implements pty_spawn: forkpty, an async-signal-safe child sequence (PDEATHSIG arm + getppid recheck on Linux, chdir, execveat(fd)/execve(path) per the plan, self-pipe failure reporting), and a parent-side bounded poll+cancel_fd handshake that never returns success for an unobserved child. start_identity is captured in the parent immediately after forkpty returns, before anything can reap/recycle the pid. Deviates from the initial draft in three places, each verified on this macOS dev box: - The child's failure-reporting helper is a top-level function, not a nested one -- GCC/Clang nested functions need trampoline support this build's plain `cc -shared` doesn't enable. - The bounded handshake retries poll()/read()/waitpid() across EINTR against a monotonic deadline, and treats a partial errpipe read or an unexpected read() failure as a bounded failure rather than guessing. - macOS's PROC_PIDUNIQIDENTIFIERINFO struct size is not the commonly documented 40/48 bytes -- empirically probed on this kernel via proc_pidinfo (self vs. a freshly forked child vs. pid 1) at 56 bytes, with the leading p_uuid/p_uniqueid/p_puniqueid prefix confirmed by the child's uniqueid incrementing from the parent's and matching parent's puniqueid. Reads only that fixed-offset prefix out of an oversized buffer instead of asserting an exact total size, so OS-version drift in the reserved tail doesn't require a code change. Also fixes the test's default execveat_supported (was hardcoded 1, which built an EXEC_FD-mode plan pty_spawn's exec step cannot run on macOS) to probe via pty_probe_execveat() like a real caller would, and swaps the "fast-exiting child" test off /bin/true (absent on this dev box) onto /bin/sleep 0.
Every Linux pty_spawn call now runs on ONE dedicated, daemon-lifetime native thread (UnixSpawnerThread) instead of a thread-pool thread, since PR_SET_PDEATHSIG is a per-thread property: a retired pool thread would otherwise SIGKILL every agent it ever spawned. Unexpected termination of the spawner thread itself Environment.FailFasts the daemon process, so children die WITH the daemon rather than being silently orphaned by a half-broken supervisor. Adds a NativeTestHost helper project (plain console Exe, no MTP/TUnit) so the PDEATHSIG-kills-child and FailFast-terminates-host assertions can run against a real, externally observable OS process; both are Linux-CI-only (PDEATHSIG has no macOS/Windows equivalent). The in-process pool-thread-churn test is portable and verified locally on macOS.
…managed fork/exec branch Rewrite UnixPtyProcess.Spawn to resolve the executable pre-fork (parent-side, against the same env/PATH the child gets), build an execution plan via pty_preflight, and submit it to the dedicated UnixSpawnerThread instead of the old forkpty + managed case-0 child branch (setenv/unsetenv/chdir/execvp, deleted entirely). The captured StartIdentity from pty_spawn's handshake is surfaced on IPtyProcess/IHostedAgentRuntime. The spawner thread is DI-owned (threaded through UnixPtyProcessFactory's constructor) rather than a static per-process singleton: UnixSpawnerThread starts a non-background OS thread, and nothing could ever reach in and Dispose() a static instance -- confirmed empirically, since a first-draft static Lazy<UnixSpawnerThread> here hung the test host indefinitely. Widen UnixSpawnerThread to public (SpawnOn stays internal) since DI's constructor-injection resolution only considers a type's public constructor, which by C#'s accessibility-consistency rule requires the parameter type to be at least as accessible. The new end-to-end spawn test's cross-check against ProcessStartToken only holds on Linux (both read /proc/pid/stat + boot_id independently in the same format); macOS's shim capture uses a different, more robust bootsessionuuid+p_uniqueid scheme that ProcessStartToken.ForPid has no knowledge of, so there is no live re-deriver to cross-check against there.
…nstead of re-capturing on Unix PersistPidRecordOrThrow gains a capturedStartIdentity parameter. Non-null (Unix, post-L1-managed(b)): consume the runtime's own IHostedAgentRuntime .StartIdentity as-is -- never re-capture via ProcessIdentity.Capture, since the shim already read it immediately post-forkpty (the capture-binding rule; a post-hoc re-capture here could adopt an unrelated process if the pid was already recycled). Empty string is a deliberate identity_unavailable record, not a launch failure, and agent.StartIdentity is set to that same empty string (not null) so CleanupAgentAsync's teardown check treats it as permanently uncomparable rather than confirmed-gone (MatchesTri(pid, "") always returns null -- no ':' scheme separator). Null (Windows / the ACP runtime, neither of which capture this way): unchanged legacy behavior, falling back to the original post-hoc ProcessIdentity.Capture. The persisted AgentPidRecord shape is intentionally left unchanged here -- an explicit identity_kind classification is a follow-up concern, not part of this rewiring.
…spawner thread retires (fixes shutdown hang)
Adds a mac-identity-smoke mode to the Task 4 NativeTestHost (captures its own pid's identity via a direct pty_capture_mac_identity call, plus a spawned child's via the production pty_spawn capture path already exercised by StartIdentity) and attaches it as an osx-arm64-only step in release.yml's per-RID build job, per the design spec's per-RID macOS packaging checks (there is no macOS runner in ci.yml to attach this to instead).
…ern.bootsessionuuid)
…rcises the real branch
… decode + inconsistent-shape quarantine
…ry; macOS legacy/identity unresolvable classification
Once AgentOrchestrator is resolved, the Unix IPtyProcessFactory has started UnixSpawnerThread's foreground (non-background) OS thread, which parks forever until the ServiceProvider is disposed. Previously only the nameInUse catch and the WaitForShutdownAsync finally called DisposeHostAsync; a throw out of ReapOrphansOnceAsync, a non-nameInUse ConnectAsync failure, CleanupOrphanedAsync, or EvalRunner resolution escaped without disposal, leaving the spawner thread parked and hanging the process instead of exiting with the error. Wrap everything from ReapOrphansOnceAsync through WaitForShutdownAsync in one try whose finally disposes daemonLock/orchestrator/connection/host on every exit path, and fold the nameInUse catch into it (return 3; unified finally does the cleanup). Return-code semantics preserved. Structural fix; backed by the existing DaemonHostDisposalTests that prove StopAsync-then-dispose retires the DI-owned spawner thread. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Job_creation_failure_fails_the_spawn_closed assigned the shared Windows test host to a UI-restricted job to block job nesting. That mutation is IRREVERSIBLE (Windows has no un-assign), so once it ran it permanently blocked later job nesting and would make Disposing_the_process_kills_child_and_grandchild / Job_sets_no_breakaway_flag... throw if scheduled after it. [NotInParallel] + ordering cannot fix an irreversible mutation. Move the assertion into a disposable out-of-process NativeTestHost (the same pattern the Linux PDEATHSIG/FailFast tests use): a new win-ui-restricted-fail-closed mode assigns ITSELF to a UI-restricted job, attempts a real ConPtyProcess.Spawn, and exits 0 iff Spawn failed closed (threw, no uncontained child), 20 if it leaked, 30 if the poison job could not be set up. The test launches that child and asserts exit code 0. Everything compiles on macOS/Linux (Windows P/Invokes guarded, the test body early-returns off-Windows); the fail-closed behavior itself is Windows-CI-verified only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Batched review fold-ins:
- csproj: replace the 'win-x64' RID equality guard with a win-prefix check
(!RuntimeIdentifier.StartsWith('win')) on the shim-compile target and its
sibling PropertyGroup/ItemGroup, so cross-publishing win-arm64/win-x86 from a
non-Windows host no longer compiles a mislabeled Linux/macOS .so for a Windows
RID. Keeps the !IsOSPlatform('Windows') conjunct.
- pty_shim.c: add compile-time _Static_assert on the vendored macOS
proc_uniqidentifierinfo prefix (size==32, offsetof(p_uniqueid)==16) so ABI
drift fails the build instead of mis-reading the identity at runtime.
- pty_shim.c: use pipe2(O_CLOEXEC) on Linux (atomic CLOEXEC — closes the
fork/exec inherit race that could stall exec-EOF into the 30s handshake
timeout and kill a healthy agent); keep pipe()+fcntl() on macOS/BSD but now
check both fcntl returns and fail closed. Define _GNU_SOURCE (Linux-only,
before the first system header) for the pipe2 declaration.
- pty_shim.c: report poll()'s errno on the non-EINTR poll-error path instead of
discarding it (diagnostic only; still HANDSHAKE_TIMEOUT).
- pty_shim.c: replace raise(SIGKILL) with kill(getpid(), SIGKILL) in the forked
child — raise() is not async-signal-safe; kill()/getpid() are. Same semantics.
- UnixPtyInterop.cs: delete the now-unused execvp/setenv/unsetenv/chdir
P/Invokes (dead since the managed fork/exec branch was removed).
Linux pipe2 arm and the win-arm64/win-x86 cross-publish are CI-verified only;
macOS path compiles clean (-Wall -Wextra) and the static_asserts hold locally.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Summary by QodoHosted-agent native OS containment: Windows Job Object, Linux PDEATHSIG shim, macOS identity
AI Description
Diagram
High-Level Assessment
Files changed (39)
|
Code Review by Qodo
1. PATH CWD mismatch
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+32
to
+40
| static string ResolveExecutableAbsolutePath(string command, string cwd, IReadOnlyDictionary<string, string> childEnv) { | ||
| if (Path.IsPathRooted(command)) return command; | ||
| if (command.Contains('/')) return Path.GetFullPath(command, cwd); | ||
|
|
||
| var path = childEnv.TryGetValue("PATH", out var p) ? p : Environment.GetEnvironmentVariable("PATH") ?? ""; | ||
| foreach (var dir in path.Split(':', StringSplitOptions.RemoveEmptyEntries)) { | ||
| var candidate = Path.Combine(dir, command); | ||
| if (File.Exists(candidate)) return candidate; | ||
| } |
There was a problem hiding this comment.
1. Path cwd mismatch 🐞 Bug ≡ Correctness
UnixPtyProcess.ResolveExecutableAbsolutePath resolves bare command names using PATH entries relative to the daemon’s current directory and drops empty PATH components, but the child does chdir(cwd) before exec. This can cause launching the wrong executable or failing to launch a command that execvp would find under the child’s cwd.
Agent Prompt
## Issue description
`UnixPtyProcess.ResolveExecutableAbsolutePath` resolves `command` by iterating `PATH.Split(':', RemoveEmptyEntries)` and doing `Path.Combine(dir, command)`, which (a) ignores empty PATH fields (which mean “current directory” on POSIX) and (b) resolves relative PATH entries against the daemon’s current working directory rather than the child’s `cwd`. Because the native child path explicitly `chdir(cwd)` before `exec*`, this changes which binary gets executed.
## Issue Context
The new Unix path resolves the executable pre-fork and then the native shim performs `chdir(cwd)` in the child before `execve/execveat`.
## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Pty/Unix/UnixPtyProcess.cs[23-43]
### Concrete fix approach
- Split PATH with `StringSplitOptions.None` (not `RemoveEmptyEntries`) so empty fields are preserved.
- For each PATH entry:
- If entry is empty: treat as `cwd`.
- Else if entry is relative: resolve with `Path.GetFullPath(entry, cwd)`.
- Else: use as-is.
- Combine the resulting base directory with `command` and only return an absolute, normalized path.
- Keep existing behavior for rooted commands and commands containing '/'.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+92
to
+97
| public void Dispose() { | ||
| _stopping = true; | ||
| _queue.CompleteAdding(); | ||
| _thread.Join(TimeSpan.FromSeconds(5)); | ||
| _queue.Dispose(); | ||
| } |
There was a problem hiding this comment.
2. Spawner dispose race 🐞 Bug ☼ Reliability
UnixSpawnerThread.Dispose only waits 5 seconds for the foreground spawner thread to exit and then disposes the BlockingCollection even if the thread is still running. Since pty_spawn can block for ~30 seconds, shutdown can be delayed and disposing the collection can trigger an exception escaping the run loop during shutdown (the outer catch is disabled when _stopping is true).
Agent Prompt
## Issue description
`UnixSpawnerThread.Dispose()` calls `_thread.Join(TimeSpan.FromSeconds(5))` and then `_queue.Dispose()` regardless of whether the thread has actually stopped. Because the spawner thread is foreground (`IsBackground = false`) and can be inside `pty_spawn` for up to ~30s, the dispose path can (1) return while the process still has a live foreground thread (delayed termination), and (2) dispose the BlockingCollection while `GetConsumingEnumerable()` is still active, potentially throwing during shutdown.
## Issue Context
- The native shim’s `pty_spawn` handshake uses a ~30 second deadline.
- The run loop wraps `GetConsumingEnumerable()` in a try/catch that only triggers when `_stopping` is false; exceptions during shutdown are not handled.
## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Pty/Unix/UnixSpawnerThread.cs[59-97]
- src/Capacitor.Cli.Daemon/Native/pty_shim.c[610-616]
### Concrete fix approach
- Ensure the thread has exited before disposing `_queue`:
- Prefer `_thread.Join()` with no timeout, or
- Join with a timeout >= native worst-case (>= 30s) and if it elapses, fail fast / throw and do not dispose `_queue`.
- Only call `_queue.Dispose()` after a successful join.
- Optionally: if you intend bounded shutdown, add a cancellation mechanism wired to `cancelFd` and signal it during dispose, so in-flight `pty_spawn` can exit promptly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+68
to
+81
| // M1-A (spec §4.3): the only rejected shapes are NEW-schema-inconsistent combinations — | ||
| // Present claiming a comparable identity with an empty token, or IdentityUnavailable | ||
| // claiming NO comparable identity while still carrying a nonempty one. A LEGACY record | ||
| // (no identity_kind key at all) always decodes as Present (PidIdentityKind's zero value) | ||
| // and is never rejected here, however old its token scheme. | ||
| var inconsistent = | ||
| (record.IdentityKind == PidIdentityKind.Present && record.StartIdentity.Length == 0) || | ||
| (record.IdentityKind == PidIdentityKind.IdentityUnavailable && record.StartIdentity.Length != 0); | ||
|
|
||
| if (inconsistent) { | ||
| logger.LogWarning( | ||
| "AgentPidRecordStore: record {Path} has an inconsistent identity_kind/start_identity combination ({Kind}, token length {Len}); quarantining as .corrupt", | ||
| path, record.IdentityKind, record.StartIdentity.Length); | ||
| TryQuarantine(path); |
There was a problem hiding this comment.
3. Null startidentity crash 🐞 Bug ☼ Reliability
AgentPidRecordStore.ReadAll dereferences record.StartIdentity.Length outside the JSON-deserialization try/catch. A parseable record with missing/null start_identity can throw NullReferenceException and abort OrphanReaper’s record pass.
Agent Prompt
## Issue description
`AgentPidRecordStore.ReadAll()` does per-record schema validation using `record.StartIdentity.Length` without first validating `StartIdentity` is non-null. Because the deserialization try/catch only catches thrown exceptions, a parseable-but-incomplete JSON shape can produce `StartIdentity == null` and then crash the entire read/sweep.
## Issue Context
`OrphanReaper.ReapOnceAsync` iterates `store.ReadAll()` directly; an exception from `ReadAll()` aborts the orphan sweep.
## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AgentPidRecordStore.cs[52-86]
- src/Capacitor.Cli.Daemon/Services/OrphanReaper.cs[30-36]
### Concrete fix approach
- Treat `StartIdentity == null` as corrupt and quarantine:
- Before computing `inconsistent`, add `if (record.StartIdentity is null) { log; TryQuarantine(path); continue; }`
- Use the local non-null variable for `Length` checks (and for logging length) to avoid multiple dereferences.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Hosted review-flow reviewers are unattended and count against the daemon's
--max-agentsbudget. Phase A (server, AI-1313) made the server reclaim leaked slots and Phase B (daemon self-defense) added the managed record/scan/quarantine backstop. This PR adds the missing layer underneath both: OS-level containment at spawn time, so a crash-surviving hosted agent is killed by the kernel wherever the OS supports it, and the managed layer only has to cover what the OS can't.What this adds
KILL_ON_JOB_CLOSEJob Object viaPROC_THREAD_ATTRIBUTE_JOB_LIST(creation-time, no breakaway): the OS kills the agent and every descendant the instant the last job handle closes — clean shutdown, daemon crash, or external kill. Launch fails closed if the job can't be created/joined. No survivor class remains to reap.libpty_shim) forks the agent, armsPR_SET_PDEATHSIG, and execs viaexecveaton a dedicated daemon-lifetime thread (PDEATHSIG binds to the creating thread's lifetime, so a pool thread would be fatal). An async-signal-safe post-fork child sequence with an unforgeable exec-EOF error-pipe handshake reports failures; agetppidre-check closes the fork/arm race. Applies to launches the shim proves contained at initial exec (non-privileged, non-deep-shebang, kernel ≥ 3.19); everything else falls back to the managed layer.mac:{bootsessionuuid}:{p_uniqueid}incarnation identity (vendored private ABI viaproc_pidinfoflavor 17 +kern.bootsessionuuid, spare-shaped fail-safe).identity_kindon the PID record distinguishes a captured identity from a capture failure;OrphanReaperclassifiesidentity_unavailable(Linux still marker-scan-recovers it) vslegacy_unresolvable/identity_unresolvable(macOS manual-kill-only, both logged each sweep, never a wrong kill).Per-OS coverage matrix (spec §4.5)
identity_unavailablerecord is still marker-scan-recovered here<3.19/probe-disabled kernel, multi-token/unresolvable shebang; post-exec privileged re-exec; group-escaped current-epoch descendantmac:{bootsessionuuid}:{p_uniqueid}incarnation identitylegacy_unresolvable/identity_unresolvablerecords (manual-kill-only)Reviews
mac:byte-parity, backward-compat JSON decode, and the Windows job binding were each confirmed correct. Two IMPORTANT findings fixed: (1)DaemonRunnernow disposes the host on every post-spawner-start exit path (a non-nameInUseConnectAsyncfailure previously parked the foreground spawner thread → hang); (2) the Windows UI-restricted-job fail-closed test was relocated out-of-process so it can't irreversibly poison the test-host process. Minor robustness items folded in: win-prefix RID guard,_Static_asserton the vendored macOS struct, Linuxpipe2(O_CLOEXEC),pollerrno reporting,raise→kill(getpid()), dead-P/Invoke cleanup.Testing & verification
kcap-cli unit suite (TUnit/MTP), isolated dummy processes only — no live daemon, no live flows. Native Linux (PDEATHSIG/
execveat/pipe2) and Windows (Job Object) behavior executes only on CI (ci.ymlubuntu+windows;release.ymlper-RID native builds compile + load-and-call-smoke each shim); the macOS-reachable parts (identity capture, handshake mechanics) run locally. The pre-CI whole-branch review is the safety net for the CI-only paths.Deferred (follow-ups, non-blocking)
Stacking
Was stacked on #327 (AI-1313 Phase B); #327 has merged, so this is rebased onto
main.