supervisor: mark deliberate severance so a probe-induced death is not a crash (#64)

[iceteaSA]

Mechanism only, ahead of the Tier-2 probe, per the ruling on #64.

A probe that induces a fatal connection state kills the module, and today that death is indistinguishable from a genuine crash at on_child_exit — ExitReport carries no reason. So the probe would consume restart budget and could push a module to Failed while measuring whether it decodes a flag bit.

The three requirements

1 — severance bound to (pid, start_time), not module id. record_deliberate_severance stores a ProcessIdentity and the child-wait path compares the exited child's identity before classifying. The race arm is the point of it:

let severed   = ProcessIdentity { pid: 41, start_time: 101 };
let successor = ProcessIdentity { pid: 41, start_time: 202 };
assert!(!module.record_deliberate_severance(severed).unwrap());
assert_eq!(exit_report.kind, ExitKind::Crash);

Same pid, different start time — the successor's death is a crash, not a laundered exemption. Same identifier-reuse discriminator as #76, reusing that PR's process_start_time rather than adding a second reader.

2 — budget/lifetime divergence, asserted in both directions. A deliberate severance increments lifetime_restarts and leaves restart_count alone. Both counters are asserted in the same test, and the crash arm asserts both move — a change that exempts everything passes the first arm while destroying the budget, so the second arm is what keeps the first honest.

3 — decode tolerance. TerminalExitKind is an open string enum with an Unknown(String) variant: an unrecognised wire value decodes to Unknown("future_exit_kind") rather than failing the record.

A note on that third requirement, because it bears on #79

Unknown(String) retains the unrecognised value rather than collapsing it. That is what makes it different from the catch-all docs/subc-control-protocol.md:275 rejects — the objection there is that a fallback makes an unknown op and a malformed body decode alike, and those warrant opposite responses. An open variant carrying the string preserves that distinction: an unknown kind arrives as Unknown("..."), while a malformed body still fails to decode.

I raise it because #79 asks the same question about RunningImageUnavailableReason, which shipped closed in #76 and breaks 0.6.0 clients on the whole supervisor.provenance response. This PR demonstrates the middle path exists and costs one variant. Whichever way you rule on #79, the two enums should end up on the same principle rather than differing by which PR they arrived in.

Platform limitation — states plainly, because the probe author needs it

process_start_time reads /proc/<pid>/stat and returns None off Linux. No identity means no severance marker, so on macOS and Windows a probe-induced death is classified Crash and consumes restart budget.

That is the correct failure direction — fail closed, never grant an exemption we cannot substantiate — and it matches the house rule that an unconfirmable identity forfeits the claim. But it is a real constraint on the probe: on the macOS fleet, Tier-2 adoption checking spends budget. Worth knowing before the probe is designed rather than discovering it when a module hits Failed.

A macOS path would need a different start-time source (sysctl KERN_PROC_PID / kinfo_proc.ki_start); out of scope here.

Naming

DeliberateSeverance names the mechanism — the daemon attests that it severed the connection, not why. The doc comment says so, so a future non-probe deliberate severance either reuses it honestly or adds its own variant instead of borrowing a probe-shaped label.

Verification

cargo test --workspace                    779 passed  0 failed  1 ignored
cargo clippy --workspace --all-targets -D warnings   clean
cargo fmt --all --check                   clean
cargo build --workspace --locked          clean
check-wire-crate-versions.sh origin/master  6 crates examined, none unbumped

Baseline on 6cf83890 is 775, not the 774 I briefed — 775 + 4 new tests = 779. The implementer measured 779, flagged that it could not reproduce my number, and reported the measurement instead of adopting the brief. My error, caught by refusing to launder it.

Red proofs, one per arm: deliberate branch spending budget → expected 0 got 1 · crash branch omitting the increment → expected 1 got 0 · identity match removed → stale-marker assertion failed · deliberate kind serialised as crash → expected deliberate_severance got crash.

Versions: subc-control 0.8.0, subc-client-rs 0.10.0, subc-core 0.10.0. One golden fixture updated — client_control_response_supervisor_terminals.json now pins exit_kind: crash on real-handler terminal records, which is the pre-existing behaviour made explicit rather than a change to it.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes probe-induced module deaths being classified as crashes: a probe that kills a module via fatal connection severance now gets a deliberate severance marker, so the death no longer consumes restart budget.

• Deliberate severance is bound to (pid, start_time) so a successor process with a reused PID is still treated as a crash.
• A deliberate severance increments lifetime_restarts but leaves restart_count unchanged.
• The new exit_kind wire field is additive and decodes unknown future kinds as Unknown(String) instead of failing the record.
• Generic fatal connection teardown cannot arm the marker, so a surviving process can't carry a stale exemption into a later genuine crash.
• On macOS and Windows, process_start_time is unavailable, so severance cannot be verified and probe-induced deaths still count as crashes and spend restart budget — fail closed.

Consumer impact

• ck module terminals now prints the exit kind.
• Package versions bumped: subc-control 0.8.0, subc-client-rs 0.10.0, subc-core 0.10.0.

^{Written for commit 317db83. Summary will update on new commits.}

[cubic-dev-ai]

All reported issues were addressed across 14 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[iceteaSA]

Valid finding — fixing it. One clause of the stated consequence does not hold, and the difference changes what the fix has to do, so recording both.

The gap is real

apply_deliberate_severance_marker has exactly one production call site (supervise.rs:2671, in supervise_loop after active_child.wait()), but there are four other reap paths:

2635  active_child.wait()   supervise_loop                      marker applied
3109  .wait()               reload_child                        no marker
3896  child.wait()          wait_for_registration_after_reload  no marker
4093  child.wait()          drain_child_to_state                no marker
4118  child.wait()          drain_child_to_state                no marker

drain_child_to_state is reached from drain_optional_child, which has seven callers across the operator and health paths. A severed module racing a health check gets reaped there instead of in supervise_loop. Reachable, not theoretical.

Correction: the drain path does not consume restart budget

I read drain_child_to_state end to end. It sets state.state, writes state.last_exit, and calls record_terminal. It never touches restart_count. So "deliberate severance will consume restart budget" is not what happens on this path.

The two defects that are there:

1. The terminal record is mislabelled. record_terminal and state.last_exit both receive ExitKind::Crash for a death the daemon deliberately caused. That is worse than a missing record — it actively attributes our kill to the module. The whole point of the label, per the ruling, is that the operator keeps the record that we killed their module; a wrong label defeats that more thoroughly than an absent one, because nothing prompts anyone to look further.

2. lifetime_restarts is not incremented either. Requirement 2 says a deliberate severance increments lifetime (it happened) and spares budget (we caused it). Reaped via drain it increments neither — the divergence breaks in the direction that loses the event entirely.

So the corrected consequence is: a probe-induced death reaped by the drain path is recorded as a crash and leaves no lifetime trace — a provenance defect rather than a budget one. Same severity, different fix.

What the fix has to be careful about

The obvious repair — mark every drain — would be wrong. drain_optional_child also serves ordinary operator stop and disable, where incrementing lifetime_restarts is incorrect: the operator stopped it, nothing restarted. So the lifetime increment has to be conditional on the deliberate-severance classification, and there is a test arm for exactly that: an ordinary operator stop through the same path must record its normal disposition and leave lifetime alone. A fix that passes the severance arm while corrupting every operator stop is the failure mode here.

I am also auditing all four unmarked sites rather than the two named, and requiring a per-site verdict on whether a severed child can reach it — a path that provably cannot see one does not need the marker, but that has to be stated per site rather than skipped silently.

The (pid, start_time) identity comparison stays load-bearing at every new application site. Marker consumption by module id alone would reintroduce the successor-labelling bug that severance_marker_for_a_dead_child_does_not_label_its_successor pins.

Thanks for the catch — this is the same class as the finding on #76: a classifier correct on the path it was written for, and absent from the paths that reach the same state by another route.

[iceteaSA]

Fixed at 4c86649d. Centralised rather than patched per-site, and the audit turned up a fifth site the review did not name.

The fix

classify_reaped_child_exit(snapshot, child, status) now runs at every point an ExitReport is constructed from a wait result:

2645  supervise_loop                        was the only marked site
3119  reload timeout reap
3901  wait_for_registration_after_reload
4100  drain_child_to_state (timeout wait)
4131  drain_child_to_state (post-kill wait)

Centralised deliberately: a future reap path inherits classification instead of having to remember it. The defect was a classifier living on one path while four other paths reached the same state by another route — patching the two named sites would have preserved the shape that produced it.

All five audited for reachability rather than the two reported. The post-kill wait at 4131 was not in the finding and is reachable by the same route as 4100, sharing the identity-bound helper so the marker is consumed exactly once.

Lifetime conditionality — the part that could have gone wrong

state.last_exit = Some(exit_report.clone());
if exit_report.kind == ExitKind::DeliberateSeverance {
    state.lifetime_restarts += 1;
}

drain_optional_child also serves ordinary operator stop and disable, where a lifetime increment would be plain wrong — the operator stopped it, nothing restarted. So the increment is gated on the classification, not on the path. restart_count is untouched throughout, preserving the divergence the ruling requires.

Two arms, and the second is the load-bearing one:

drain_reap_marks_deliberate_severance_and_records_lifetime_without_budget
ordinary_drain_reap_does_not_record_a_lifetime_restart

RED evidence: with the classifier absent the deliberate drain observed Some(Crash); with the increment made unconditional the ordinary drain reported 1 instead of 0. A fix that marks every drain passes the first arm while corrupting every operator stop, so the second arm is what holds the first honest.

One deliberate behaviour change beyond the finding

registration_failure_exit_report rewrites a replacement child's exit to Crash on the reasoning that a process exiting before HELLO did not provide service, even on status 0. It now exempts DeliberateSeverance:

if exit_report.kind != ExitKind::DeliberateSeverance {
    exit_report.kind = ExitKind::Crash;
}

Without this, a severed replacement would be relabelled a new-binary failure — the same mislabelling, one layer up. A genuine new-binary failure carries no marker and still becomes Crash, because the exemption requires the identity-bound marker rather than a state check. Flagging it since it is a change to pre-existing logic, not new code.

Verification

cargo test --workspace                              781 passed  0 failed  1 ignored
cargo clippy --workspace --all-targets -D warnings  clean
cargo fmt --all --check                             clean
cargo build --workspace --locked                    clean
check-wire-crate-versions.sh origin/master          6 crates, none unbumped

781 = 779 + the 2 new arms. Existing arms unchanged, including genuine_crash_spends_restart_budget_and_records_lifetime and severance_marker_for_a_dead_child_does_not_label_its_successor — the successor-race arm still pins that identity binding, which had to hold at every new application site or the fix would have reintroduced exactly the bug that arm exists to catch.

[cubic-dev-ai]

1 issue found across 14 files (changes from recent commits).

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/server.rs">

<violation number="1" location="crates/subc-core/src/server.rs:441">
P2: The deliberate-severance marker is recorded on every fatal routing failure for a module connection, including non-probe data connections where the module process can survive the connection teardown. Because `snapshot.deliberate_severance` persists until the next reap of a matching (pid, start_time) or a reset, a module that outlives the severance and later exits for a genuine reason is classified as `DeliberateSeverance`, masking a real crash and skipping restart-budget consumption. The record should be scoped to the probe/severance path that is actually followed by process death, or the marker should be cleared when the process is observed still alive after the connection close.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: The deliberate-severance marker is recorded on every fatal routing failure for a module connection, including non-probe data connections where the module process can survive the connection teardown. Because snapshot.deliberate_severance persists until the next reap of a matching (pid, start_time) or a reset, a module that outlives the severance and later exits for a genuine reason is classified as DeliberateSeverance, masking a real crash and skipping restart-budget consumption. The record should be scoped to the probe/severance path that is actually followed by process death, or the marker should be cleared when the process is observed still alive after the connection close.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-core/src/server.rs, line 441:

<comment>The deliberate-severance marker is recorded on every fatal routing failure for a module connection, including non-probe data connections where the module process can survive the connection teardown. Because `snapshot.deliberate_severance` persists until the next reap of a matching (pid, start_time) or a reset, a module that outlives the severance and later exits for a genuine reason is classified as `DeliberateSeverance`, masking a real crash and skipping restart-budget consumption. The record should be scoped to the probe/severance path that is actually followed by process death, or the marker should be cleared when the process is observed still alive after the connection close.</comment>

<file context>
@@ -438,6 +438,7 @@ where
                     error = %err,
                     "fatal routing failure"
                 );
+                router.record_deliberate_connection_severance(ctx.connection_id);
                 return Err(ConnectionError::Router(err));
             }
</file context>

[iceteaSA]

Valid, and it is the exact failure direction this design was chosen to avoid. Fixing.

Confirmed at source

server.rs:441 sits on the generic fatal-routing-failure branch:

} else {
    debug!(connection_id = ..., error = %err, "fatal routing failure");
    router.record_deliberate_connection_severance(ctx.connection_id);
    return Err(ConnectionError::Router(err));
}

That is every fatal routing failure on a module connection, not only a deliberate severance. Many do not kill the module process.

The identity binding does not protect against this, and I want to be precise about why, because it protects against something adjacent. (pid, start_time) exists to stop a marker labelling a successor's death — different process, different start time, no match. Here the module survives the connection teardown and the same process later dies for a real reason. Identity matches exactly, because it is genuinely the same process. The marker applies to a death we did not cause: a real crash rendered as a deliberate act, restart budget spared for a module that is actually failing.

This is the option I argued against, arrived at from the other side

On #64 I rejected marking intent in side state in these words:

a stale intent silently exempts a real crash, and a guard whose failure direction is silent-permissiveness is the class this repo keeps refusing

The maintainer ruled (b) partly on that reasoning. The implementation then reproduced the same defect by a different route — a marker that outlives the event it describes. Choosing the right architecture does not confer immunity from the failure mode that architecture was chosen to avoid; the marker just had to be set somewhere, and the somewhere was too broad.

It is also strictly worse than the mislabelling fixed last round. That one attributed our kill to the module; this one attributes the module's own failure to us, and spares the budget that would have caught it.

Direction

Narrowing the record site rather than clearing the marker afterwards. The invariant: a marker must never be settable by a path that does not know the severance was deliberate. A generic routing error that is fatal to the connection is not a deliberate severance, and the party that decided to sever is the party that knows — so the marker belongs with the initiator, not inferred from a teardown branch.

Clear-if-still-alive is available as a backstop but is weaker on its own: it depends on an observation window and has a race. Not setting it has neither. If the deliberate path turns out not to be distinguishable at that call site, that changes what this mechanism can promise and I will say so on this PR rather than quietly substituting the weaker form.

The fix arm is written to fail first: a module connection takes a fatal routing failure, the process survives, and a later genuine crash must classify as Crash with budget consumed. That has to go RED against 4c86649d before the fix, or the defect has not been reproduced.

Good catch. Two rounds, two real findings, both the same underlying shape — a mechanism correct at the point it was written and wrong about the paths that reach it.

[iceteaSA]

Fixed at 317db835, direction (i). Two things about it I want to state plainly rather than let a reader assume.

The fix

The generic fatal-routing branch no longer arms anything — that call and its forwarding/control/router plumbing are gone. The marker is armed only by an explicit initiator:

/// must not call this:
/// a surviving process would otherwise retain an exemption for a later
/// genuine crash.
pub fn record_deliberate_severance(&self, module_id: &str) -> Result<bool, SuperviseError> {
    ...
    module.record_deliberate_severance(ProcessIdentity { pid, start_time })
}

The invariant is now structural rather than conventional: a path that cannot know the severance was deliberate has no way to arm the marker. The initiator captures (pid, start_time) at the moment it decides to sever, and classification still requires exact identity equality.

Defect reproduced before fixing, per your point: restoring the old call made fatal_connection_teardown_cannot_arm_a_marker_for_a_surviving_process fail. Removing it turned the arm green.

Weakness in that regression test — flagging rather than presenting it as strong

The guard is a source-text assertion:

assert!(!include_str!("server.rs")
    .contains("router.record_deliberate_connection_severance(ctx.connection_id)"));

It catches a literal reintroduction of that exact call, which is the likely regression, and it is cheap. It does not catch a semantically identical reintroduction through a renamed helper, a different receiver, or a reformatted call. It is a tripwire, not a proof.

The behavioural form — fatal routing failure, process survives, later genuine crash classifies as Crash with budget consumed — needs a live daemon and module and belongs in the real_daemon suite. I did not build it here because the mechanism has no production caller yet (below), so there is nothing to drive the severance in a live scenario. When the probe lands and there is a real initiator, that arm becomes writable and should replace this one. Happy to note it as a follow-up on #64 if you want it tracked rather than left to the probe PR.

The mechanism is currently dormant, deliberately

Nothing in production calls record_deliberate_severance. Every caller today is a test. That is correct for a mechanism landing ahead of its probe — but it means no code path can currently produce ExitKind::DeliberateSeverance in a running daemon, and I would rather say so than have it read as live.

Consequence for review: the risk this PR carries is not "does the classification fire correctly in production" — it cannot fire yet. It is "is the mechanism correct and unable to fire wrongly", which is what the last two rounds were about. The first probe caller is the point at which the behaviour becomes observable.

Verification

cargo test --workspace                              782 passed  0 failed  1 ignored
cargo clippy --workspace --all-targets -D warnings  clean
cargo fmt --all --check                             clean
cargo build --workspace --locked                    clean
check-wire-crate-versions.sh origin/master          6 crates, none unbumped

Existing arms unchanged and passing: deliberate classification, genuine crash spending budget, the successor-race identity binding, and ordinary drains staying lifetime-neutral.

Three rounds, three real findings, and all three were the same shape — a mechanism correct where it was written and wrong about which paths reach it. Round 1: classifier on one reap path, four others unmarked. Round 2: marker armed by a branch that could not know the severance was deliberate. The narrowing each time was toward fewer places allowed to assert the fact, which is where it should have started.

[cubic-dev-ai]

1 issue found across 11 files (changes from recent commits).

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/supervise.rs">

<violation number="1" location="crates/subc-core/src/supervise.rs:778">
P1: The new deliberate-severance marker is never armed in production, so probe-induced child deaths are still classified as crashes and consume restart budget. Invoke this API from the actual daemon-initiated severance path, while keeping generic connection teardown out of it.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: The new deliberate-severance marker is never armed in production, so probe-induced child deaths are still classified as crashes and consume restart budget. Invoke this API from the actual daemon-initiated severance path, while keeping generic connection teardown out of it.

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 778:

<comment>The new deliberate-severance marker is never armed in production, so probe-induced child deaths are still classified as crashes and consume restart budget. Invoke this API from the actual daemon-initiated severance path, while keeping generic connection teardown out of it.</comment>

<file context>
@@ -743,6 +770,22 @@ impl SupervisorHandle {
+    /// must not call this:
+    /// a surviving process would otherwise retain an exemption for a later
+    /// genuine crash.
+    pub fn record_deliberate_severance(&self, module_id: &str) -> Result<bool, SuperviseError> {
+        let Some(module) = self.get(module_id) else {
+            return Ok(false);
</file context>

[iceteaSA]

Accurate on the fact, and I disclosed it myself in the comment above — but it is the ruled scope of this PR, not a defect in it. Declining, with the check I ran before declining.

The ruling scoped it this way

From #64:

Ruling: (b), as its own PR ahead of the probe

The mechanism lands first; the probe is the next PR. Dormancy is the intended state of this change, not an oversight in it — which is why I stated it plainly rather than leaving it to be found.

But "no production caller" deserved a check, not a citation

The finding says to invoke the API "from the actual daemon-initiated severance path". That presumes such a path exists today. I went looking, because if the daemon already deliberately severs a module connection in a way that kills the process, arming there would be legitimate and the ruling would not cover it.

It does not:

/// True only when an undeliverable GOODBYE should escalate to closing the
/// target connection. Never escalate for module recipients.
pub(crate) fn close_on_delivery_failure(&self) -> bool {
    matches!(self.kind, GoodbyeTargetKind::Client)
}

Module-targeted GOODBYEs close routes, not connections, and explicitly never escalate to closing the connection. The drain and reload paths take ownership of the process lifecycle directly — they do not sever a connection and wait for the module to die from it, which is the same finding that started #64: the operator paths avoid the budget by owning the lifecycle, not by severing.

So there is no existing daemon-initiated, process-killing severance to wire this to. The first one will be the probe. Wiring it to any path that does not actually kill the process is precisely the P2 fixed one round ago — a marker armed by something that cannot know the severance was fatal, left to exempt a later genuine crash.

What would change my mind

If you want the mechanism to land with its first caller rather than ahead of it, that is a reasonable position and I will build the probe into this PR — but it is a scope change to a ruled decision, so it is @ualtinok's call rather than mine to take from a review comment. Say the word and I will fold it in.

Worth noting the three rounds together, because the trajectory is consistent: round 1 narrowed classification to every reap path, round 2 narrowed arming to an initiator that knows the severance was deliberate, and this would widen arming again to a path that does not. The first two moved toward fewer places allowed to assert the fact. I would rather leave the mechanism unarmed than arm it somewhere that cannot honestly make the claim.

[subc-alfonso]

Review by execution against the three #64 requirements, all three landing exactly — plus the #79 ruling applied ahead of its own paper trail:

1. Identity binding: marker is (pid, start_time), one-shot take(), equality-gated — and the mismatch direction fails TOWARD crash (budget consumed), which is the correct failure polarity: a lost label costs an operator a wrong record; a loose label would cost the budget its meaning. The mismatch test at the reaped-wrong-process arm pins it.
2. Two-sided counter divergence: severance arm asserts lifetime_restarts == 1 && restart_count == 0 with the crash control asserting both increment — both polarities, so the variant silently decaying to crash reddens by name.
3. Wire tolerance: TerminalExitKind as an open string enum with Unknown(String) beats the serde(other) floor I asked for — the unknown retains its wire spelling for rendering — and Option + default covers absent-on-old-daemons. Golden updated, both directions.

Naming verdict: DeliberateSeverance is the mechanism-honest choice per the ruling — the attestation is the severance, and the doc comment saying generic teardown must not arm it is the guard against label borrowing. The bot's never-armed-in-production P1 is by design (mechanism-only ahead of the probe, the #64 sequencing); the arming call is the probe PR's first line. Gate: 469/0 locally, full matrix green on the head. Merging.