Repository navigation
Conversation
…ortexkit#103) Re-lands cortexkit#103 onto the post-split layout. The original branch predates the subc-core/subc-daemon split, so its rebase could not resolve: master's subc-core/src/lib.rs is now a binary package marker, and re-adding the module list there would declare modules that no longer live in that crate. The seven files are re-applied by hand instead, and the daemon-side shutdown drain (stamp_shutdown + drain_for_daemon_shutdown + the second-SIGTERM path) is preserved -- the old branch's bootstrap hunk would have deleted it. When OMP starts the daemon it sets OMP_SUBC_OWNED=1, and from then on the monitor is the ONLY thing that retires the tree: the extension deletes its own lease on shutdown and nothing else. A lease directory that loses its last live holder is the trigger, which is what covers a forced close, where no session_shutdown callback runs at all. Retirement order is drain-then-kill, per review: a module hangs graceful-stop work off the GOODBYE the drain delivers (broca seals its WAL, engram closes a capture), so killing the tree first turns that delivery into a no-op against a dead process and can kill a module mid-write. Two corrections to the original: * The tree kill captured its pid AFTER drain_optional_child had taken the child, so it always read None and silently never ran. The pid is now read before the drain. * A tree-kill failure no longer fails the module: it is logged at warn and the drain result still decides the outcome, so a module that drained cleanly is never reported as failed because taskkill could not reach a helper. retire_tree is additive -- ordinary retire passes tree: false and keeps its exact previous behaviour, so nothing that worked before changes. The lease spec lands at docs/specs/subc-lease-contract.md: lease files, processIdentity, the fail-closed rules, retirement ordering and the stranded lock recovery. subc-daemon 0.20.8: version bump for the wire check.
f6959eb to
303345e
Compare
|
Thank you for the careful re-application. The corrections you found are real, and the dead tree-kill (the pid read after We're still not taking the lifecycle change, though, and I want to correct one thing in the description so the history is accurate: #103 was not closed on the mechanical blockers. Those were in an earlier review round, and your revisions fixed them. It was closed on scope, in the closing comment:
Gating it behind On the grandchild kill: your #111 is the version we want. A job object with suspended creation and no breakaway contains the tree by kernel membership, which covers the reparented-grandchild case you correctly list as One process note, as with #103: CONTRIBUTING.md asks for an issue before a PR that changes daemon behaviour, and this one would have been settled in a sentence there. I'm closing this PR; please don't read that as a judgement on the work, which was careful. |
What this does
Re-lands #103 onto the current layout. That branch was closed on a gate that named three mechanical blockers (fmt, the
retire_treedead-code gate on non-Windows, and alet _retired =unit bind) and it has since gone stale: it was cut before thesubc-core/subc-daemonsplit, so it cannot be rebased.A straight rebase does not resolve.
crates/subc-core/src/lib.rson master is now a binary package marker —//! Binary package marker; the daemon library is provided by `subc-daemon`.— so re-applying the branch's module list there would declare modules that no longer live in that crate. Its
bootstrap.rshunk also competes rather than adds: it would delete master'sstamp_shutdown()/drain_for_daemon_shutdown()/ second-SIGTERM path.So the seven files are re-applied by hand onto the post-split tree, and the daemon-side shutdown drain is preserved.
The mechanism
When OMP starts the daemon it sets
OMP_SUBC_OWNED=1. From that point the monitor is the only thing that retires the tree: the extension deletes its own lease on shutdown and nothing else acts.A lease directory that loses its last live holder is the trigger. That is what covers a forced close — a killed host runs no
session_shutdowncallback at all, which is the gap the current design leaves open.Safety properties kept from the reviewed original:
Ordering: drain, then kill
Per review, the tree kill runs after the drain-and-wait. A module hangs graceful-stop work off the GOODBYE the drain delivers — broca seals its WAL, engram closes a capture — so killing the tree first turns that delivery into a no-op against a dead process and can kill a module mid-write.
Two corrections to the original
drain_optional_childhad already taken the child, so it always readNone. The pid is now read before the drain. This is the finding that matters most — the original's tree kill was dead code that would have passed a review read.warnand the drain result still decides the outcome, so a module that drained cleanly is never reported as failed becausetaskkillcould not reach a helper.retire_treeis additive: ordinaryretirepassestree: falseand keeps its exact previous behaviour, so nothing that worked before changes.A third correction, found by review after the first push: the
Retirearm returned a barefalse, where the reviewed original returnedtree && registration_released. That bool drives the caller'sif !handle_supervisor_command(..) { break; }, so withfalsea tree retirement never ended the module's supervision loop — the restart policy stayed armed and the daemon could respawn a module the holder monitor had just retired, which is the orphan leak this change exists to close.Honest limits
taskkill /Tis not complete containment. It walks current parent-child relationships, so a grandchild whose parent already exited has been reparented out of the tree and is invisible to it. That case is what #111's job-object containment covers; this is the belt to that braces. Landing subc-daemon: job-object containment for module grandchildren (#109) #111 first would let this rely on it.spawn_if_ownedreturnsNoneelsewhere, and the kill is#[cfg(windows)].OMP_SUBC_OWNED=1andOMP_SUBC_STOP_ON_LAST_EXITis not0.tick_oncetests use a scriptedProcessProbeagainst a realSupervisorHandleand a temp run dir. They cover the lock choreography and the fail-closed rules; they do not cover a real forced close end to end.Verification
cargo test -p subc-daemon --lib— 302 passed, 0 failed (299 before, +3 fromholder_monitor)cargo test -p subc-daemon --lib holder_monitor— 6 passed, 0 failedcargo clippy -p subc-daemon --all-targets -- -D warningscleancargo fmt --all -- --checkcleansubc-daemonbumped to 0.20.8 for the wire checkThe lease contract lands at
docs/specs/subc-lease-contract.md: lease files,processIdentity, the fail-closed rules, retirement ordering, and stranded-lock recovery.