Conversation
cdc230e to
f89abdf
Compare
![subc-alfonso[bot]](https://avatars.githubusercontent.com/u/271195168?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the shape #109 asked for, and it gets the hard part right.
What I checked. Each child is created suspended, assigned to its job, then resumed, so it can't run a single instruction outside containment. That spawn-to-assign window is the one that decides whether a job object contains anything at all. Breakaway is refused, so a module can't opt out with CREATE_BREAKAWAY_FROM_JOB. The unsafe lives in its own leaf crate behind a safe API, so subc-daemon keeps its lint. The supervisor tests go through the drain path instead of stopping at the crate, which is the claim that matters: a crate test proves a job can reap a tree, not that the daemon's teardown reaches it. Gate on this branch: fmt clean, clippy clean on host and on x86_64-pc-windows-gnu in both profiles, a Linux cross-check clean, and the workspace at 1454 passed / 0 failed. The Windows-only tests themselves run only in CI, since I have no Windows host in this gate.
One tradeoff to write down before merge. JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE is what makes containment survive a daemon crash, and that's the right reason to set it. It also fires on every ordinary daemon stop, because on Windows the daemon has no stop notice: the SIGTERM handler is #[cfg(unix)], and a Windows stop is taskkill or the scheduler's /End. When the process ends, the kernel closes the job handle and every module is TerminateProcess'd at once.
Before this PR the modules survived that, saw EOF on their control socket, and ran their own teardown. On Unix that's deliberate: the daemon exits with process::exit(0) precisely so a module can seal a WAL or close a capture rather than being killed mid-write. So on Windows this trades graceful module teardown on every daemon stop for containment on a crash.
I think that's the right trade for now. The orphaned GPU workers are a reported, recurring problem; mid-write kills on a Windows daemon stop are a smaller one, since the modules that write most heavily don't run on Windows today. But it should be explicit, not incidental. Could you add a short paragraph to the job field's doc saying so, and naming the fix, so the next reader doesn't take the crash rationale as the whole story?
The fix is mine, not yours. Give the Windows daemon a real stop path that drains before it exits, the Windows twin of the Unix SIGTERM handler. Once that exists, KILL_ON_JOB_CLOSE only reaches whatever is left after the drain, which is exactly what it should reach. That's the daemon's stop contract, so I'll take it as a follow-up and link it here.
With the doc paragraph added, I'm happy to merge.
The job field's doc explained the limit only as crash containment. It also fires on every ordinary Windows daemon stop, because the daemon has no stop notice there: the SIGTERM handler is #[cfg(unix)], so a stop is taskkill or the scheduler's /End, the kernel closes the job handle, and every module is TerminateProcess'd at once rather than running its own teardown. Stating the trade and naming the fix, so the next reader does not take the crash rationale as the whole story. Requested in review of cortexkit#111.
|
#109 has the |
…ortexkit#109) A supervised module may spawn helpers of its own -- the Synapse embedding module spawns a CUDA worker that holds the GPU allocation -- and nothing in teardown took them. Child::kill is TerminateProcess scoped to one pid, and Windows has no process group to signal, so a module terminated rather than asked could not close its own pipes and its grandchildren outlived it. A day of restarts accumulated orphans. New leaf crate subc-jobobject follows the subc-cgroup model: this crate forbids unsafe code and the Win32 calls need it. The job carries JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and breakaway is deliberately not permitted, so a daemon crash reaps the tree that no supervisor code is left alive to kill, and a child cannot escape by CreateProcess with CREATE_BREAKAWAY_FROM_JOB. Spawn is suspend -> assign -> resume, in contain_spawned_child: assignment must happen before the child can run a single instruction, or it could spawn a grandchild that escapes. An assignment failure is NOT fatal -- an uncontained module behaves exactly as before -- and is logged at warn. start_kill terminates the job tree after the drain wait (the maintainer's requirement): the drain and the reaped child run first, the tree kill after. Containment never changes whether a module is reported as stopped. Tests run against the supervisor: teardown reaps the grandchild, an uncontained grandchild survives a direct child kill (the defect reproduction from cortexkit#109), and dropping containment reaps the tree with no teardown code running (crash durability). The regression test was verified to redden by name under a containment-disabled mutation. The job object is the Windows arm only. Unix containment is the process group plus the child roster, which is already on master; the two compose and use independent process attributes on Linux (setpgid in the child, cgroup.procs via pre_exec). Cargo.lock gains subc-jobobject, subc-daemon takes it as a target-cfg windows dependency, and subc-core's fake-aft-stub gains the grandchild mode the containment tests drive.
The job field's doc explained the limit only as crash containment. It also fires on every ordinary Windows daemon stop, because the daemon has no stop notice there: the SIGTERM handler is #[cfg(unix)], so a stop is taskkill or the scheduler's /End, the kernel closes the job handle, and every module is TerminateProcess'd at once rather than running its own teardown. The Windows stop path is still outstanding; the Unix arm now has one (process groups plus the child roster). Stating the trade and naming the fix, so the next reader does not take the crash rationale as the whole story. Requested in review of cortexkit#111.
c6e7dee to
a4720c9
Compare
|
Doc paragraph added, on the
The Windows tests, which your gate could not run. Ran on a real Windows host: The daemon lib suite runs 348 passed / 0 failed alongside them, with One thing worth knowing about running them by hand, since it fails in a way that reads like a containment defect: the fixture needs Rebased onto master. The Unix arm landed while this was in review ( The rebase forced one integration change: The Windows stop path is the half I have not touched, as you said it is yours. |
|
Doc paragraph and rebase are pushed; the design gate is now the only blocker and it needs your label. The PR description gained the template's #109 carries no labels yet. The design conversation did happen there — the job-object ruling, and the scope narrowed to the grandchild kill — so labelling it is the last step on my side of this. Two notes on the rebased branch, since they are the parts a reviewer would otherwise have to reconstruct:
|
The job field's doc explained the limit only as crash containment. It also fires on every ordinary Windows daemon stop, because the daemon has no stop notice there: the SIGTERM handler is #[cfg(unix)], so a stop is taskkill or the scheduler's /End, the kernel closes the job handle, and every module is TerminateProcess'd at once rather than running its own teardown. The Windows stop path is still outstanding; the Unix arm now has one (process groups plus the child roster). Stating the trade and naming the fix, so the next reader does not take the crash rationale as the whole story. Requested in review of #111.
…n test drain_child_to_state gained a StopNotice argument on master after #111 was written; the test only compiles on Windows, so it was the one call site left.
The job field's doc explained the limit only as crash containment. It also fires on every ordinary Windows daemon stop, because the daemon has no stop notice there: the SIGTERM handler is #[cfg(unix)], so a stop is taskkill or the scheduler's /End, the kernel closes the job handle, and every module is TerminateProcess'd at once rather than running its own teardown. The Windows stop path is still outstanding; the Unix arm now has one (process groups plus the child roster). Stating the trade and naming the fix, so the next reader does not take the crash rationale as the whole story. Requested in review of #111.
…n test drain_child_to_state gained a StopNotice argument on master after #111 was written; the test only compiles on Windows, so it was the one call site left.
|
Thanks. This is merged to master as your two commits, with four small commits of mine on top, so I'm closing the PR here rather than asking you to rebase again. Master had moved since your last push, and a few things only showed up once the branch sat on the current code:
CI is green on every leg, including Windows. The doc paragraph on |
Linked issue
Approved issue: #109
The agreed scope on that issue is the grandchild kill only. The session/host ownership model (maintainer-owned) is untouched.
What this does
Contains a supervised module's whole process tree in a Windows job object, so teardown reaches helpers a direct-child kill cannot.
A supervised module may spawn helpers of its own — the Synapse embedding module spawns a CUDA worker that holds the GPU allocation. Nothing in teardown took them:
Child::killisTerminateProcessscoped to one pid, and Windows has no process group to signal, so a module terminated rather than asked could not close its own pipes and its grandchildren outlived it. A day of restarts accumulated orphans.This is the Windows arm only. Unix containment is the process group plus
child_roster(already on master); the two are platform-disjoint and compose at the spawn site.Why a job object rather than a tree walk
A job contains by membership, not ancestry, which covers the two cases a
taskkill /Twalk cannot:Two properties are load-bearing:
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE— dropping the last handle kills every member. That is what makes containment survive a daemon crash, where no code runs to call anything.JOB_OBJECT_LIMIT_BREAKAWAY_OKunset is what stops a child from escaping withCreateProcess/CREATE_BREAKAWAY_FROM_JOB. A module that could leave the job could leave its grandchildren behind, which is the defect.The suspend → assign → resume contract
Assignment must happen before the child can run a single instruction, or it could spawn a grandchild that escapes — the defect in smaller form.
CreateProcesshas no way to place a process in a job atomically short of building the process by hand withPROC_THREAD_ATTRIBUTE_JOB_LIST, so the child is created suspended, assigned, and then resumed.There is exactly one place that order lives (
contain_spawned_child), and the crate root documents it.resume_main_threadreports failure rather than logging it: a suspended process holding a pid with no way to start is a leak, so the caller kills it and fails the spawn.An assignment failure is not fatal — an uncontained module behaves exactly as it did before, whereas refusing to start one would be a new outage. It is logged at
warnbecause it means a helper could leak.Ordering at teardown
start_killterminates the job tree, and it runs at the existing kill site — after the drain budget and after the reaped child, never before. Containment never changes whether a module is reported as stopped: a job failure is logged and the direct-child kill still decides the outcome.New crate
subc-jobobjectfollows thesubc-cgroupmodel: a leaf crate, becausesubc-daemonforbids unsafe code and the Win32 calls need it. Every FFI call and its safety argument is in one file (sys.rs), and the manualSendfor the job handle sits on that side of the boundary rather than on the public API.Syncis deliberately not implemented — concurrentTerminateJobObjectandDropon one handle would be a double-close.Tests
Three, run against the supervisor rather than the crate, because the claim is about teardown: a crate-level test proves a job can reap a tree, not that the daemon's drain path reaches it.
teardown_reaps_the_grandchildan_uncontained_grandchild_survives_a_direct_child_killdropping_containment_reaps_the_grandchildThe mutation control was run: with assignment disabled,
teardown_reaps_the_grandchildreddens by name —The fixture is the sanctioned
fake-aft-stub, extended with a grandchild mode rather than a new binary.subc-coreandsubc-daemonare version-bumped for the wire check.Verification
cargo test -p subc-daemon --lib— 299 passed, 0 failedcargo test -p subc-jobobject— 4 passed, 0 failed (+ doc test)cargo clippy --all-targets -- -D warningsclean on both cratescargo fmt --all -- --checkcleanmaster(f193a178)Limits
#[cfg(windows)]arm compiles out elsewhere; the cgroup lane is a separate containment path with its own tests.start_killstill calls it, so nothing about existing stop reporting moves.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Contains a supervised module's whole Windows process tree in a job object, so teardown reaches grandchildren a direct-child kill cannot. A module's CUDA worker held the GPU allocation and outlived restarts, accumulating orphans.
subc-jobobjectcrate; the child is created suspended, assigned, then resumed so nothing runs before it is contained.KILL_ON_JOB_CLOSEalso fires on every ordinary Windows stop (taskkill or scheduler/End, no SIGTERM handler), killing all modules at once instead of letting them run teardown — accepted for now; a real drain-before-exit stop path is the planned fix.subc-coreandsubc-daemonare version-bumped.Written for commit a4720c9. Summary will update on new commits.