Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/subc-core/src/holder_monitor.rs">
<violation number="1" location="crates/subc-core/src/holder_monitor.rs:50">
P2: When a lease has a null or missing `processIdentity`, `owner_gone` still treats a gone PID as proof that the holder is gone. This violates the fail-closed lease contract and can retire the daemon while an untrustworthy lease remains; require a non-empty identity before accepting any owner as gone.</violation>
</file>
<file name="crates/subc-core/src/supervise.rs">
<violation number="1" location="crates/subc-core/src/supervise.rs:3021">
P1: When tree retirement has active forwarding routes, `taskkill` disconnects the module before `begin_forwarding_drain_if_configured` can send `route.closing`. Clients can therefore receive crash `route.closed`/GOODBYE without the required closing notification; start the forwarding drain before killing the process tree, while still killing the tree before waiting on the child.</violation>
<violation number="2" location="crates/subc-core/src/supervise.rs:3026">
P2: If the supervised parent exits just before this command runs, `taskkill` returns non-success and retirement aborts before the child is drained. After the supervisor reaps that parent, `retire_tree` can no longer target its surviving grandchildren, so use a race-safe tree ownership/termination mechanism (such as a Windows Job Object) rather than treating the parent PID as sufficient.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let result = async { | ||
| if tree { | ||
| #[cfg(windows)] | ||
| if let Some(pid) = child.as_ref().and_then(SupervisedChild::id) { |
There was a problem hiding this comment.
P1: When tree retirement has active forwarding routes, taskkill disconnects the module before begin_forwarding_drain_if_configured can send route.closing. Clients can therefore receive crash route.closed/GOODBYE without the required closing notification; start the forwarding drain before killing the process tree, while still killing the tree before waiting on the child.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-core/src/supervise.rs, line 3021:
<comment>When tree retirement has active forwarding routes, `taskkill` disconnects the module before `begin_forwarding_drain_if_configured` can send `route.closing`. Clients can therefore receive crash `route.closed`/GOODBYE without the required closing notification; start the forwarding drain before killing the process tree, while still killing the tree before waiting on the child.</comment>
<file context>
@@ -2998,8 +3014,32 @@ async fn handle_supervisor_command(
let result = async {
+ if tree {
+ #[cfg(windows)]
+ if let Some(pid) = child.as_ref().and_then(SupervisedChild::id) {
+ let mut command = Command::new("taskkill.exe");
+ command.args(["/PID", &pid.to_string(), "/T", "/F"])
</file context>
| fn owner_gone(owner: &Owner, state: &ProcessState) -> bool { | ||
| if owner.pid == 0 || owner.token.is_empty() { return false; } | ||
| match state { | ||
| ProcessState::Gone => true, |
There was a problem hiding this comment.
P2: When a lease has a null or missing processIdentity, owner_gone still treats a gone PID as proof that the holder is gone. This violates the fail-closed lease contract and can retire the daemon while an untrustworthy lease remains; require a non-empty identity before accepting any owner as gone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-core/src/holder_monitor.rs, line 50:
<comment>When a lease has a null or missing `processIdentity`, `owner_gone` still treats a gone PID as proof that the holder is gone. This violates the fail-closed lease contract and can retire the daemon while an untrustworthy lease remains; require a non-empty identity before accepting any owner as gone.</comment>
<file context>
@@ -0,0 +1,229 @@
+fn owner_gone(owner: &Owner, state: &ProcessState) -> bool {
+ if owner.pid == 0 || owner.token.is_empty() { return false; }
+ match state {
+ ProcessState::Gone => true,
+ ProcessState::Live(actual) => owner.process_identity.as_ref()
+ .is_some_and(|recorded| !recorded.is_empty() && recorded != actual),
</file context>
| ProcessState::Gone => true, | |
| ProcessState::Gone => owner.process_identity.as_ref().is_some_and(|identity| !identity.is_empty()), |
| command.args(["/PID", &pid.to_string(), "/T", "/F"]) | ||
| .stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()) | ||
| .kill_on_drop(true).creation_flags(0x0800_0000); | ||
| let status = timeout(Duration::from_secs(10), command.status()) |
There was a problem hiding this comment.
P2: If the supervised parent exits just before this command runs, taskkill returns non-success and retirement aborts before the child is drained. After the supervisor reaps that parent, retire_tree can no longer target its surviving grandchildren, so use a race-safe tree ownership/termination mechanism (such as a Windows Job Object) rather than treating the parent PID as sufficient.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-core/src/supervise.rs, line 3026:
<comment>If the supervised parent exits just before this command runs, `taskkill` returns non-success and retirement aborts before the child is drained. After the supervisor reaps that parent, `retire_tree` can no longer target its surviving grandchildren, so use a race-safe tree ownership/termination mechanism (such as a Windows Job Object) rather than treating the parent PID as sufficient.</comment>
<file context>
@@ -2998,8 +3014,32 @@ async fn handle_supervisor_command(
+ command.args(["/PID", &pid.to_string(), "/T", "/F"])
+ .stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null())
+ .kill_on_drop(true).creation_flags(0x0800_0000);
+ let status = timeout(Duration::from_secs(10), command.status())
+ .await
+ .map_err(|_| SuperviseError::Kill {
</file context>
![subc-alfonso[bot]](https://avatars.githubusercontent.com/u/271195168?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this — the forced-close and graceful sequences you ran are exactly the verification a lifecycle change needs, and the lock ordering section is careful work. Read cold against the diff; findings below, in the order I would want them resolved. One is a design question I am taking to the operator in parallel, since it decides whether this mode exists at all; the rest stand on their own.
1. taskkill /T /F runs BEFORE the drain, which makes the drain a no-op on a dead process.
In the Retire { tree: true } arm, the tree kill executes and only then does begin_forwarding_drain_if_configured run. Teardown order in this daemon is drain → route.closed pushes → per-route GOODBYEs → module GOODBYE → wait up to 30 s for the child's own exit → kill. Modules hang graceful-stop work off that GOODBYE (broca seals its WAL on it; engram closes a capture); a /F first kills them mid-write. "No holders left" does not mean "no module work in flight" — those loops run on their own schedule, not on client sessions. The /T is the right tool for grandchildren, but it belongs at the kill step, replacing start_kill() after the drain-and-wait, not ahead of it. Please move it and add a test that retirement with a live drain still delivers the module GOODBYE before the process dies (mutation: swap the order back → test reds by name).
2. The lock choreography is the load-bearing claim and has no automated coverage.
The one test added exercises owner_gone as a pure function. Nothing tests the monitor loop, the two leases_unchanged re-verifies, the stranded-lock reap, or the retirement sequence; the "host registers mid-retirement aborts it" property — the reason the double re-verify exists — is asserted in prose and verified by hand on one machine. That is the class of claim I will not take on a description. Please add tests at the loop level with a fake process_state (the probe is the only Windows-bound piece): (a) all holders gone → retires; (b) a lease file appears between probe and boundary lock → aborts, nothing retired; (c) a lease appears between boundary and operation lock → aborts; (d) an unparseable lease blocks; (e) a stranded lock with a dead owner is reaped, with a live owner is not. Each with a mutation named (e.g. delete the second re-verify → (c) reds).
3. The probe shells to PowerShell every 2 s for the life of the daemon.
process_state spawns powershell.exe + Get-CimInstance per lease per tick, up to 5 s each, serialized. On the Windows VM I drive, PowerShell start alone is ~5 s cold and >1 s warm; a background PowerShell every two seconds on every OMP-owned daemon is a real CPU and battery cost that scales with holder count. OpenProcess + GetProcessTimes via the windows crate answers "is pid alive and is its creation time X" with no shell and no CIM. Not a blocker for correctness (fail-closed on Unknown is right), but I would not ship the shell form.
4. The lease contract is defined nowhere on this side.
OMP_SUBC_OWNED, OMP_SUBC_STOP_ON_LAST_EXIT, subc-lease-*.json with {pid, token, processIdentity}, subc-retiring.lock, the writer's wx/250 ms/20 s lock discipline — none of this exists in the repo outside this diff, and the writer lives in the OMP extension. If this merges, subconscious owns a file-format contract with a third-party writer and no spec. It needs docs/specs/ — writer and reader obligations, the identity format, what a torn or stale lease means — before the code, not after. What is OMP, concretely (the harness, the extension, where it lives)? That changes how the spec is written.
5. Mechanics before merge: base is 5a89716c, master is past 0.17.48; rebase, and bump subc-core (the wire check refuses unbumped changes). holder_monitor.rs is compiled on all platforms and gated at runtime by cfg!(windows) — prefer #[cfg(windows)] on the module so non-Windows builds do not carry PowerShell command strings.
The design question, stated so it is not hidden in the review: this makes the daemon a second lifecycle — session-scoped and harness-owned, retiring when the last editor closes — beside the one it was built as (a user-scoped singleton registered by ck setup as a logon task on Windows, on which broca, engram, and plexus do work whether or not an editor is open). The leak you fixed exists because the OMP extension spawns its own daemon. The structural alternative is that OMP never spawns one and uses the service ck setup registers, and then there is nothing to retire. Whether subc should support a harness-owned mode at all is the operator's call, not mine; I am asking. If the answer is yes, items 1–5 are what it takes; if no, the retire_tree grandchild fix is still worth landing on its own, at the kill step.
A supervised daemon started by the OMP lifecycle extension now retires itself when its last holder exits, including holders that die without emitting session_shutdown (taskkill /T /F, crash, terminal close). holder_monitor.rs is gated on OMP_SUBC_OWNED=1 plus Windows, so standalone, launchd, and test daemons keep their existing behaviour. Lock ordering: lease fingerprints are read and probed unlocked; the registrar lock is taken only once every holder is gone, and it reaps a stranded lock itself (read, probe, re-read, remove, create_new) before two leases_unchanged re-verifies gate retirement. This keeps host registration responsive while the CIM probes run. supervise.rs gains retire_tree(): capture the child pid before retire() destroys the handle, then taskkill /T /F the tree so grandchildren die with the module. bootstrap.rs wires the monitor into the serve select. Regressions: forced-close (first-kill preserves, last-kill retires daemon+module+grandchild, abandoned registrar lock recovered), graceful (cold start, shared owners, first-exit preserve, last-exit retire, restart after retirement), 276 lib tests, strict clippy.
… seam, loop tests, spec - retire path drains and waits before taskkill /T /F, so the GOODBYE the drain delivers reaches a live module instead of a dead process - process_state behind an object-safe ProcessProbe trait; the loop is extracted to tick_once so the lock choreography has coverage off-Windows - five loop-level tests with an injected probe: live holder blocks, all-gone retires and removes the discovery file, a holder registering after the probe aborts, unprobeable fails closed, empty leases retire - holder_monitor is #[cfg(windows)]; non-Windows builds keep it out - docs/specs/subc-lease-contract.md: the lease, lock, classification, retirement ordering, and stranded-lock recovery contract
347391d to
b2312f7
Compare
|
Replied to each finding; design question answered in-thread. Done in this push (rebased onto master, v0.17.49):
3. Native probe — deliberately not done in this push. The trait seam makes it a one-function swap ( Design question — I'll build whichever you rule, here's my read: Honest case for keeping the monitor in the harness-owned mode this PR adds: the harness already owns the daemon's lifecycle (it starts it, it sets The thing that would change my mind: if you want OMP hosts to stop launching daemons at all and have the service serve every host, then the leases and the monitor belong to the service and most of this PR goes away in favor of that. That's a better architecture if it's where you're heading — it removes the startup race and the lock entirely. I did not build it because it is a larger change than the defect in front of me, and the forced-kill leak is real today either way. One correction to my own earlier claim: I said 276 tests passed pre-rebase; the post-rebase run is 281 passing with the 6 monitor tests, and 26 pre-existing failures on clean upstream master ( |
There was a problem hiding this comment.
3 issues found across 6 reviewed files. 1 file intentionally excluded from review.
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/specs/subc-lease-contract.md">
<violation number="1" location="docs/specs/subc-lease-contract.md:117">
P3: §5 claims a freshness gate the implementation does not have. `take_lock()` in holder_monitor.rs reaps a stranded lock as soon as its recorded owner probes dead: it reads the bytes, probes the owner pid via `owner_gone`/`process_state`, re-reads for change, and removes — no timestamp, mtime, or 30-second age check exists anywhere. `Owner` carries only `pid`, `token`, and `process_identity`, so a lock written milliseconds earlier with a dead owner is removed immediately. The claim also contradicts §4's own chevron (`read → probe → re-read → remove → create_new`). This is a contract spec, so the false timing promise can mislead maintainers reasoning about reaping races.</violation>
</file>
<file name="crates/subc-core/src/holder_monitor.rs">
<violation number="1" location="crates/subc-core/src/holder_monitor.rs:330">
P2: This test does not exercise a holder registering after the probe or validate the boundary-lock re-verification. Add the second lease from the probe callback after the initial snapshot, and return `Gone` for both PIDs so retirement reaches the re-check and aborts there.</violation>
</file>
<file name="crates/subc-core/src/bootstrap.rs">
<violation number="1" location="crates/subc-core/src/bootstrap.rs:594">
P3: On non-Windows builds the `#[cfg(not(windows))]` branch fixes `holder_task` to a constant `None`, which makes the immediately following `if let Some(task) = holder_task.as_mut()` guard always false. The whole embedded `tokio::select!` — including the `task.join()` retirement branch and its `drop(serve_task)`/`drop(_watchdog_task)`/`return Ok(())` path — is therefore unreachable dead code that will never execute on any non-Windows build. Gate that block with `#[cfg(windows)]` and let the non-Windows build fall straight through to `serve_task.join().await`, so the unreachable select is compiled out instead of being carried as always-false dead code.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let dir = fx.dir.path().to_owned(); | ||
| let rt = Runtime::new().unwrap(); | ||
| // Register the new holder before the tick so the re-verify sees it. | ||
| write_lease(&dir, 1001, "id-b"); |
There was a problem hiding this comment.
P2: This test does not exercise a holder registering after the probe or validate the boundary-lock re-verification. Add the second lease from the probe callback after the initial snapshot, and return Gone for both PIDs so retirement reaches the re-check and aborts there.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-core/src/holder_monitor.rs, line 330:
<comment>This test does not exercise a holder registering after the probe or validate the boundary-lock re-verification. Add the second lease from the probe callback after the initial snapshot, and return `Gone` for both PIDs so retirement reaches the re-check and aborts there.</comment>
<file context>
@@ -203,14 +188,185 @@ impl HolderMonitor {
+ let dir = fx.dir.path().to_owned();
+ let rt = Runtime::new().unwrap();
+ // Register the new holder before the tick so the re-verify sees it.
+ write_lease(&dir, 1001, "id-b");
+ let retired = rt.block_on(tick_once(&dir, "self", &connection, &fx.supervisor, &probe));
+ assert!(retired.is_none());
</file context>
| 3. re-read; if the bytes changed meanwhile, someone re-took it, leave it | ||
| 4. remove, then `create_new` — the reaper is now the lock owner | ||
|
|
||
| Too-fresh locks (< 30s) are left alone, and any probe ambiguity fails closed |
There was a problem hiding this comment.
P3: §5 claims a freshness gate the implementation does not have. take_lock() in holder_monitor.rs reaps a stranded lock as soon as its recorded owner probes dead: it reads the bytes, probes the owner pid via owner_gone/process_state, re-reads for change, and removes — no timestamp, mtime, or 30-second age check exists anywhere. Owner carries only pid, token, and process_identity, so a lock written milliseconds earlier with a dead owner is removed immediately. The claim also contradicts §4's own chevron (read → probe → re-read → remove → create_new). This is a contract spec, so the false timing promise can mislead maintainers reasoning about reaping races.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/specs/subc-lease-contract.md, line 117:
<comment>§5 claims a freshness gate the implementation does not have. `take_lock()` in holder_monitor.rs reaps a stranded lock as soon as its recorded owner probes dead: it reads the bytes, probes the owner pid via `owner_gone`/`process_state`, re-reads for change, and removes — no timestamp, mtime, or 30-second age check exists anywhere. `Owner` carries only `pid`, `token`, and `process_identity`, so a lock written milliseconds earlier with a dead owner is removed immediately. The claim also contradicts §4's own chevron (`read → probe → re-read → remove → create_new`). This is a contract spec, so the false timing promise can mislead maintainers reasoning about reaping races.</comment>
<file context>
@@ -0,0 +1,162 @@
+3. re-read; if the bytes changed meanwhile, someone re-took it, leave it
+4. remove, then `create_new` — the reaper is now the lock owner
+
+Too-fresh locks (< 30s) are left alone, and any probe ambiguity fails closed
+to "leave it". Reaping happens only when the monitor is about to retire anyway,
+never on a routine tick, so a live fleet never sees its lock touched.
</file context>
| Too-fresh locks (< 30s) are left alone, and any probe ambiguity fails closed | |
| Too-fresh locks are not age-gated: a lock whose recorded owner probes dead is removed regardless of age, and a live matching owner, a changed file, or any probe ambiguity leaves it alone. |
| bound.connection_file_path.clone(), supervisor_handle, | ||
| ).map(AbortOnDrop::new); | ||
| #[cfg(not(windows))] | ||
| let mut holder_task: Option<AbortOnDrop<()>> = None; |
There was a problem hiding this comment.
P3: On non-Windows builds the #[cfg(not(windows))] branch fixes holder_task to a constant None, which makes the immediately following if let Some(task) = holder_task.as_mut() guard always false. The whole embedded tokio::select! — including the task.join() retirement branch and its drop(serve_task)/drop(_watchdog_task)/return Ok(()) path — is therefore unreachable dead code that will never execute on any non-Windows build. Gate that block with #[cfg(windows)] and let the non-Windows build fall straight through to serve_task.join().await, so the unreachable select is compiled out instead of being carried as always-false dead code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-core/src/bootstrap.rs, line 594:
<comment>On non-Windows builds the `#[cfg(not(windows))]` branch fixes `holder_task` to a constant `None`, which makes the immediately following `if let Some(task) = holder_task.as_mut()` guard always false. The whole embedded `tokio::select!` — including the `task.join()` retirement branch and its `drop(serve_task)`/`drop(_watchdog_task)`/`return Ok(())` path — is therefore unreachable dead code that will never execute on any non-Windows build. Gate that block with `#[cfg(windows)]` and let the non-Windows build fall straight through to `serve_task.join().await`, so the unreachable select is compiled out instead of being carried as always-false dead code.</comment>
<file context>
@@ -584,9 +586,12 @@ async fn serve_bound_daemon(
bound.connection_file_path.clone(), supervisor_handle,
).map(AbortOnDrop::new);
+ #[cfg(not(windows))]
+ let mut holder_task: Option<AbortOnDrop<()>> = None;
if let Some(task) = holder_task.as_mut() {
tokio::select! {
</file context>
|
Read the push cold and ran it through the merge gate here. The substantive items landed the way I asked — drain-and-wait before the tree kill with the reason in a comment, the probe behind a trait with the loop extracted, five loop-level tests against the real Gate results on
On the 26 "pre-existing failures on clean upstream master": master is green on every CI leg at On the monitor tests: they are Design question: still with the operator, and your read is a fair statement of the harness-owned case — per-run-dir, per-instance, dies with its lease set, so retirement is the tail of a chain the harness already commands. I have put that read in front of him beside the service alternative. Fix the three mechanical items and I will cut the twin so CI's Windows leg runs the choreography tests while he decides; nothing else on the code is blocking. |
Problem
A supervised daemon started by the OMP lifecycle extension was retired by the extension itself on the last host's
session_shutdown. When that hook never fires — taskkill, a crash, a terminal close — the daemon, its supervised modules, and their grandchildren all leaked, surviving every host.Change
The daemon now owns its own retirement.
holder_monitor.rs(new) — ticks every 2s, gated onOMP_SUBC_OWNED=1and Windows, so standalone, launchd, and test daemons are untouched. When every lease holder is gone it retires the supervised trees, removes the connection file, and exits the process.supervise.rs—retire_tree(): captures the child pid beforeretire()destroys the handle, thentaskkill /T /Fso grandchildren die with the module instead of surviving it.bootstrap.rs— the monitor task joins the serveselect, so retiring the daemon tears the listener and watchdog down cleanly.Lock ordering
Lease fingerprints are read and probed unlocked. The registrar lock is taken only once every holder is gone, and it reaps a stranded lock itself (read → probe owner liveness → re-read to confirm no change → remove →
create_new). Twoleases_unchangedre-verifies then gate retirement — one under the boundary lock, one under the operation lock — so a host that registers mid-retirement aborts it.This matters because every host must win that same lock (
wx, 250ms retry, hard 20s throw) to register or release a lease. Probing under the lock would block host registration for up to one 5s CIM probe per lease per tick on slow WMI.Fail-closed throughout: an unparseable lease, a failed probe, or a null identity reads as a live holder and blocks retirement.
Verification
tests/watchdog.rsintegration green,clippy --all-targets -D warningsclean.Regressions run against a freshly built
ck-subc.exein isolated run directories; the production daemons were not restarted for this PR.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes leaked OMP daemons by moving last-host retirement from the lifecycle extension's
session_shutdownhook into the daemon itself. Previously a supervised daemon survived forever when that hook never fired (taskkill, crash, terminal close); now the daemon drains and retires its supervised trees, removes its connection file, and exits once every lease holder is gone.Bug Fixes
holder_monitor.rsticks every 2 s, gated on Windows andOMP_SUBC_OWNED=1;OMP_SUBC_STOP_ON_LAST_EXIT=0opts out, so standalone, launchd, and test daemons are untouched.retire_tree()drains beforetaskkill /T /F, so graceful-stop work reaches a live process and grandchildren die with the module.docs/specs/subc-lease-contract.mddocumenting the lease, lock, and retirement contract.Written for commit b2312f7. Summary will update on new commits.