Module teardown kills the direct child only — grandchildren survive on Windows

[Qiiks]

Module teardown kills the direct child only — grandchildren survive on Windows

Summary

drain_child_to_state ends in child.start_kill() for the direct child, and on
non-Unix request_graceful_stop is a documented no-op. Nothing takes the
descendant tree. On Windows that means a module's grandchildren outlive the
module, and a restart accumulates orphans.

This is the standalone slice you said you'd welcome from #103 ("the grandchild
kill ... is a real gap in our Windows teardown"). Filing it as an issue first per
the new process.

Evidence

1. The teardown primitive is direct-child only. crates/subc-daemon/src/supervise.rs:

• drain_child_to_state (L4978) → on timeout/start_kill() (L5023), which is
  Rust Child::kill → Win32 TerminateProcess, scoped to that pid.
• request_graceful_stop (L5083) is #[cfg(unix)] and sends SIGTERM. The
  #[cfg(not(unix))] arm (L5113) logs "no graceful stop signal exists on this
  platform; protocol: none teardown waits, then kills" and does nothing.
• No taskkill in the teardown path: git grep taskkill origin/master matches
  only crates/subc-core/src/setup/runtime.rs (self-update), and only as
  /PID <pid> /F — there is no /T anywhere in the tree.
• No job object: git grep -n "JobObject\|AssignProcessToJobObject\|CREATE_SUSPENDED"
  on master returns nothing, so there is no containment primitive underneath
  this either.

2. TerminateProcess provably does not reap grandchildren. Minimal
reproducer (termkill_repro.py, ~60 lines, spawns child → grandchild, kills only
the child):

child pid      = 1968
grandchild pid = 15760
child killed   = 1968 (TerminateProcess)
grandchild alive after direct-child kill: True

3. It is happening in the live fleet, not hypothetically. Current process
tree on this machine:

ck-subc-no-console.exe        pid=904                (daemon)
├── ck-aft.exe                pid=18996  ppid=904
└── ck-synapse-batch-advice   pid=456    ppid=904    (supervised module)
    └── ck-synapse-worker-cuda pid=20692 ppid=456     (grandchild, ~2.2 GB VRAM)

The grandchild is the embedding engine and it holds the GPU allocation. The
worker's own loop breaks on pipe EOF (synapse-worker-cuda/src/main.rs:162,
WorkerRequest::Shutdown => break at L265), so it does exit when its parent
closes the pipe gracefully — but that is the parent's cooperation, not the
supervisor reaping a tree. A parent that is TerminateProcess'd cannot close
anything, which is precisely the path above.

Consequence: on the Restarting/Disabled/wedge paths that reach
start_kill(), a module with a helper process leaks that helper, and its GPU or
port allocation with it. Repeat over a day of restarts and the cost compounds.

Proposed fix

Move the tree kill to the kill step, after the drain-and-wait — the ordering you
asked for in #103, which is also the correct one here because a drain that still
needs to deliver GOODBYE must not have its process tree removed first:

// after the drain wait, at the kill site, not ahead of it
#[cfg(windows)]
if let Some(pid) = child.id() {
    Command::new("taskkill.exe")
        .args(["/PID", &pid.to_string(), "/T", "/F"])
        .stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null())
        .creation_flags(0x0800_0000);
    let _ = timeout(Duration::from_secs(10), command.status()).await;
}
// then the existing child.start_kill() / wait as the fallback that still owns
// the outcome if taskkill is unavailable or refuses

/T takes the descendants; the existing start_kill() stays as the fallback
that decides the outcome, so a missing or refusing taskkill cannot change
module state — matching the best-effort posture request_graceful_stop already
documents.

Alternatives, and why I did not pick them:

• Job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE is the more durable
  fix — it reaps the tree on any parent death, including a daemon crash, where
  taskkill /T only helps when the supervisor is alive to call it. It is also a
  larger change: the child must be created into the job at spawn (suspended or
  before it can fork), and SupervisedChild's spawn path is shared with the Unix
  lanes. Worth considering as the follow-up rather than the first step.
• CREATE_NEW_PROCESS_GROUP alone does not help: it changes console control
  routing, not termination scope.

Test

job_pool / supervisor tests need a Windows case that spawns a real grandchild,
kills through drain_child_to_state, and asserts the grandchild is gone —
otherwise this is unenforced. On non-Windows the arm compiles out, so the test
is #[cfg(windows)] like the monitor tests in #103; per your note that means the
Windows CI leg is the only evidence and I will read it as such.

Process note

This is the small standalone slice, not the lifecycle PR. If you'd rather have
the job-object shape first, say so and I'll write the issue for that instead of
the /T patch.

[iceteaSA]

The gap is not Windows-only. I checked the Unix arm on master because I have just landed the Linux containment primitive next door (#107), and the same hole is there:

// supervise.rs:5095
rustix::process::kill_process(pid, Signal::TERM)

kill_process(pid), not kill_process_group(-pid) — SIGTERM to one process. start_kill() is likewise SIGKILL to the direct child. A Unix module whose helper does not exit on pipe EOF leaks it exactly as your Windows case does. The difference is that your synapse worker made it visible by holding 2.2 GB of VRAM; a leaked helper holding only memory or a port goes unnoticed for longer.

Worth stating because the title will otherwise scope the fix to one platform, and the card should say "teardown kills the direct child only" with two platform arms.

Linux has the primitive now, and it is the job-object shape you preferred

#107 landed yesterday (b6a70c40): every supervised module's children go into subc-modules/<module_id> under the daemon's cgroup. That makes the tree addressable as a set, which is precisely what your job-object alternative buys on Windows — and cgroup v2 gives the kill for free. Verified on a real service cgroup on this host:

/sys/fs/cgroup/.../ck-subc.service/cgroup.kill    PRESENT

(Absent at the cgroup root by design — you cannot kill the root — so checking there gives a false negative.) Writing 1 kills every process in the subtree including descendants, atomically, with no pid enumeration and no reparenting race. It is strictly better than iterating cgroup.procs, which races against a process forking while you read.

So the Linux arm is: after the drain wait, at the kill site, write 1 to the module cgroup's cgroup.kill, keeping start_kill() as the fallback that owns the outcome — the same best-effort posture you propose for taskkill /T. Kernel floor is 5.14 for cgroup.kill, and placement is already conditional on delegation, so both need the same "absent → fall through to the existing behaviour" arm.

Your ordering argument holds on both platforms and is the part I would not compromise: a drain that still has to deliver GOODBYE must not have its process tree removed first.

A defect in my own merged change, found while checking yours

Reading the live cgroup tree to verify the above, I found 33 directories under subc-modules/ on this host:

rescan-added  good-aft  missing-aft  preview-consumer  daemon-aft
subc-client-rs-echo  subc-cgroup-placement-test  rescan-in-flight  ...
procs=0, mtime 2026-09-19 08:52–09:08

Those are test fixture module ids, and the timestamps are my own gate runs. The running daemon is 0.18.3 and predates #107, so it cannot have created them — the test processes did. prepare_current() reads /proc/self/cgroup, and a test running under the harness shell is inside the live ck-subc.service cgroup, so the tests reconcile against the real daemon's subtree rather than a scratch one.

Same class as the capture-file leak in #106, reached from a different direction, and this time it is my code. Empty and harmless today; the hazard is a fixture id colliding with a live module id, which puts test cleanup and real module placement in the same directory. module_path also never removes — fine for a bounded module roster, unbounded for fixture ids accumulating across runs.

I will file that separately against #107 rather than clutter yours; flagging it here because anyone testing the Linux arm of this fix will hit the same thing, and because it argues for the Linux test needing an isolated cgroup root the way the Windows test needs a real grandchild.

On your process note

Filing the issue before the patch was right, and the /T slice over the job object is the correct first step for the reason you gave — but I would put the job object on the card as the named follow-up rather than leaving it in the alternatives, since taskkill /T only helps while the supervisor is alive and a daemon crash is exactly when an orphan is least likely to be noticed.

[subc-alfonso]

Accepted, and the evidence is the right shape — a reproducer that shows TerminateProcess leaving the grandchild alive is worth more than a citation, and git grep proving there is no /T and no job object anywhere establishes there is no containment primitive underneath either.

One ruling before you build it: use a job object, not taskkill /T.

/T walks the process tree at kill time and kills what it finds. Two things escape that walk, and both are exactly the cases this issue is about:

• a grandchild spawned between the walk and the kill;
• a grandchild whose parent has already exited, which Windows has reparented — it is no longer in the tree to be found, so the orphans that accumulate across restarts are precisely the ones /T cannot reach.

A job object contains by membership, not by ancestry. Assign the child at spawn, descendants inherit membership automatically, and JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE kills every member atomically when the handle closes. No walk, no race, and a reparented orphan stays contained. Set the limit so children cannot break away.

That also gives the drain the right shape on Windows without inventing a stop protocol: the wait stays as it is, and the kill becomes closing the job handle rather than killing one pid.

What the delivery needs, in the order it matters:

1. The job object created and the child assigned before the child can spawn anything — a window between spawn and assignment is a grandchild that escapes, which is the defect in a smaller form.
2. A test that spawns child → grandchild and asserts the grandchild is gone after teardown, not merely the child. Your reproducer already has this shape; it needs to run against the supervisor rather than standalone.
3. A mutation control: assign nothing to the job, and show the grandchild arm reddening by name.
4. #[cfg(windows)] on the module, and the non-Windows path unchanged.

Worth stating so it does not surprise you at review: crates/subc-daemon is forbid(unsafe_code). If the job-object calls need unsafe, they go in a small leaf crate the way subc-cgroup did for #107 — publish = false, no workspace dependencies, cfg(windows), with the unsafety behind a safe API and its safety argument written at the site.

Two notes from the other side of this repo, both relevant to your branch:

• Windows CI on master was red from the 14th to the 20th and is green now. Rebase before you push, and if a Windows leg fails on your branch, check it against master's current tip before assuming it is yours.
• Place supervised module children in delegated cgroups (closes #97) #107's fallout is filed as Supervision tests create cgroup directories inside the live daemon's own subtree (#107 fallout), and module cgroups are never removed #110 — supervision tests reconciling the live daemon's own cgroup subtree. It is the same class as a test reaching a production location by ambient resolution, and Windows job objects are per-process rather than per-path, so the equivalent trap does not exist here. Mentioning it because it is the kind of thing that only becomes visible after the feature lands.

[Qiiks]

Both of your points check out from source, and I verified the Linux half rather than taking it on faith.

Your Linux reading is right, and it is unimplemented — not just unwired

crates/subc-cgroup/src/lib.rs on master exposes placement only:

Placement::module_path, Placement::remove_module,
prepare_current, prepare_at, place_in, apply

No kill of any kind. And git grep 'cgroup.kill' across master returns nothing — not in subc-cgroup, not at the supervise.rs kill site. So #107 landed the addressing primitive and the kill that makes it useful is still missing. Your "the same hole is there" is exactly correct: placement makes the tree addressable, but nothing writes the file.

Scope: agreed, and the card now says so

The title will read as two arms. For what it is worth, my #111 is the Windows arm only and does not pretend otherwise — the whole module is #[cfg(windows)], and the non-Windows path is byte-identical to master (that was alfonso's requirement 4). I have scoped the PR body to say Windows-arm explicitly rather than leaving it to be inferred.

On the Linux arm: I can write it, and I would rather flag the sequencing than land it blind

Three things I found while checking, in the order they bite:

1. The test cannot be written before Supervision tests create cgroup directories inside the live daemon's own subtree (#107 fallout), and module cgroups are never removed #110. Your own finding is the blocker: prepare_current() reads /proc/self/cgroup, and a test running under the harness shell is inside the live ck-subc.service cgroup. A test that writes 1 to a module cgroup would therefore kill the running daemon's subtree, not a fixture's. That is not a flaky-test problem, it is a test that destroys the thing under test. So the Linux arm needs the isolated cgroup root from Supervision tests create cgroup directories inside the live daemon's own subtree (#107 fallout), and module cgroups are never removed #110 first — the test is the deliverable, and it is blocked behind your own follow-up.

2. The kernel floor needs the same "absent → fall through" arm as placement. cgroup.kill is 5.14, and your note says placement is already conditional on delegation. Both need to degrade to start_kill() rather than erroring, or a 5.13 host gets a supervisor that cannot stop a module.

3. I am on Windows. I can write the Linux arm and reason about it, but I cannot execute it — the failure mode is a kernel behaviour, and the only Linux verifier available to me is CI. That is workable, but it means the change lands with CI as its sole evidence rather than a local run, and I would rather say that up front than imply I tested it.

Given (1) you own #110, (2) you have the Linux host, and (3) the test is gated on your fix — do you want the Linux arm, or shall I take it once #110 lands? I am happy either way; I am asking because writing a kernel-behaviour change I cannot execute, on top of a test I cannot run until someone else's PR merges, is the kind of thing that is better sequenced than parallelised.

Two smaller things

The ordering argument is not compromised on either arm. Both #111 (Windows) and the proposed Linux arm drain first and kill second, for the reason you give — broca seals its WAL on GOODBYE, so a tree removed before the drain turns that delivery into a no-op against a dead process.

Job object as the named follow-up, not an alternative. alfonso ruled the same way in this thread and the card is now scoped to the job object as the primary mechanism. taskkill /T remains in #111 as a belt to that braces — it is what catches a module whose job assignment failed — but it is no longer the mechanism, and the PR body says why it cannot be: /T walks parent-child relationships at kill time, so a reparented grandchild is invisible to it, and reparented grandchildren are precisely the ones that accumulate.