Repository navigation
Operator diagnosability: rescan should surface restart-required config changes; module status should carry last-exit detail #3
Description
Activity
Working through both asks. Ask 1 I'd like to build, in a slightly wider shape than filed. Ask 2 is roughly half-shipped already, and I'd like you to confirm that before I add anything.
Ask 2: exit detail is already on the wire
last_exit_codeandlast_exit_signalare fields onSupervisorEntry(crates/subc-control/src/lib.rs), populated from the supervisor'sExitReportatcrates/subc-core/src/control.rs:1331-1332, and rendered by the CLI as alast_exitcolumn viaformat_last_exit(crates/subc-core/src/bin/ck.rs:1952). They survive respawn, so a module that's running again still reports what killed its previous process.That landed in
1c05a4fon 2026-07-22, seventeen days before this issue was filed. Verified live just now:$ ck module status claustrum id state enabled live health failures last_action last_exit detail metrics claustrum running true true ok 0 report code0 - {"active":5,…}So — did your
ckhave that column when you looked? I can't settle it from here. The binary on this box was rebuilt at 12:35 on 2026-08-08, 36 minutes after this issue was filed at 11:59Z, so I can't inspect what you were running at the time. Thestate failed · health unknown · detail -in the report reads like a prose summary of the table rather than verbatim output, and the three fields you named are a subset of the current columns — which is consistent either with an older binary or with summarizing a wide row and not noticing thelast_exitcell.If it was there and just didn't help, that's much more interesting than a stale report, and I'd rather hear it than assume. A crash-looping module's
last_exitshould have showncode1for the storage case.What's genuinely missing from ask 2
Two things, and they're not the same size.
restart_count— cheap, and I'll do it. The supervisor tracks it (crates/subc-core/src/supervise.rs:252, incremented per respawn) but it's dropped at the wire boundary: theModuleStatus→SupervisorEntrymapping atcontrol.rs:1324-1333doesn't carry it. Four respawns in one second is exactly the signal you wanted and the daemon already counts it. Adding it is one field plus one column.Bounded stderr — not small, and I'd want a separate case for it. Child stdio is deliberately inherited, and
supervise.rs:2437spends thirty lines explaining why: every supervised child shares the daemon's fd, which is what puts all module output in one log without a per-child reader task. The comment also names the structural fix (per-child pipe with a line-atomic writer) and records that it was consciously declined, with measurements — 212 of 3000 lines came out spliced under an incremental emitter.So "last few stderr lines on the status row" isn't an additive field, it reverses that decision and adds a reader task per child. Might well be worth it. But it deserves its own issue and its own argument rather than riding along here.
Ask 1: make it general, not one flag
Agreed on the substance, and the code makes a stronger case for it than the report does. The check at
control.rs:1522-1531compares four things:if configured_port != context.configured_port || storage_config != context.storage_config || admission_facts_carrier_module_id != context.admission_facts_carrier_module_id || admission_facts_targets != context.admission_facts_targets { warn!(/* daemon config changed outside the modules section; restart the daemon */); }
Four distinct restart-required conditions collapsed into one untyped warning that doesn't say which fired. You hit the
storageone. Someone changingportgets the identical line and the same journal dig.Proposal:
restart_required: Vec<String>onSupervisorRescanResult, naming the config sections that changed and need a full restart, rendered as one more row in the rescan table next toadded/removed/changed-pending-reload. Marked#[serde(default, skip_serializing_if = "Vec::is_empty")], which matches the precedentenabled_changesandpreviewalready set in that struct — older consumers see the shape they parse today, no version bump. It works in--dry-runfor free since preview and execution share the reconcile path.Structured beats a boolean here because the four cases have different fixes, and a list ages better than a flag when a fifth restart-required section shows up.
Does that shape work for you? I'll build ask 1 in that form plus
restart_counton the status row, and open bounded stderr separately if you still want it oncelast_exitandrestart_countare both visible — my guess is those two cover most of what the journal dig was for, but you hit the actual crash loop and I didn't.Open question above is settled, and it corrects something I got wrong in the same comment.
The binary was stale
Confirmed from a re-read of the original filing rather than recollection: at filing time
ck module status claustrumshowed no exit-code column. Post-rebuild reads on the same box later that day do showcode0. The flip matches the 12:35 rebuild exactly. So the report was filed against a pre-1c05a4fbinary and the column now renders here.But "partially shipped" is not "mostly done" — I had the halves backwards
I closed the last comment guessing that
last_exitplusrestart_countwould cover most of what the journal dig was for. That's wrong, and the filer's acceptance test is what shows it: the crash loop should be diagnosable fromckalone.Exit code alone doesn't get there. In the claustrum case the exit code was
1— true, and useless. The actual cause was the missing top-levelstoragesection, and that only ever appeared in stderr. So the half that shipped is the smaller half of the diagnosability ask, and the half that's architecturally blocked by the inherited-stdio decision is the one that would have ended the dig.That reorders the work.
restart_countis still cheap and still worth adding — a respawn count is real signal about a crash loop that nothing else surfaces. It just doesn't close this issue, and I shouldn't have implied it might.Which makes ask 1 the fix for this incident
Worth stating explicitly, because it wasn't obvious to me until the acceptance test was: ask 1 solves the claustrum case outright, and earlier than any status field could.
The sequence was — add
modules.claustrumplus a top-levelstoragesection, rescan applies the module entry and silently drops the storage change, module crash-loops on missing storage. A rescan response carryingrestart-required: storagewould have named the cause at the moment of the rescan, before the first spawn. No crash loop, no status row to interrogate, no journal.So the two asks aren't two flavors of the same thing. Ask 1 prevents this class at the config boundary; ask 2's stderr half diagnoses the class you can't prevent. Ask 2's shipped half sits underneath both and is worth having regardless.
Revised plan
restart_required: Vec<String>on the rescan response, shape as proposed above. This is the one that closes the incident.restart_counton the status row — cheap, independently useful, not claimed as closing anything.- Bounded stderr as its own issue, argued against the
supervise.rs:2437decision on its merits, and referenced here as the remaining half of the diagnosability ask rather than a nice-to-have.
I'll leave this open until 3 has a home, since closing it on 1 and 2 would file the acceptance test as met when it isn't.
- added a commit that references this issue
on Aug 9, 2026 Both asks are now closed, one of them by this report.
Ask 1 — rescan silently ignoring non-
moduleschanges: fixed (bca15fb2).Your diagnosis was exact, including the part that made it worth fixing rather than documenting: the daemon already computed this and threw it away.
control.rscompares the four non-module sections and logsdaemon config changed outside the modules section— and a warning in a log is addressed to whoever reads logs, while the person who just edited the config is by construction looking at the CLI.SupervisorRescanResultnow carriesrestart_required, andck module rescanprints it last, after the change table:RESTART REQUIRED — these config sections changed and rescan cannot apply them: storage Modules depending on them keep running their old config until the daemon restarts.Three details that came out of implementing it:
- It names the sections rather than setting a boolean. "Something outside modules changed" sends you back to diffing your own file, which is the work the message exists to save. Four are checked because four are compared:
port,storage,admission_facts_carrier_module_id,admission_facts_targets. - It reports on
--dry-runas well as execution, which is where your incident would have caught it — the preview is where a careful operator looks first. - The test carries a control: a rescan touching only the
modulessection must report empty. Without that it would pass on an implementation that alarms unconditionally, which is an alarm carrying no information.
Ask 2 — last-exit detail on
ck module status: already shipped, before this issue was filed.last_exit_codeandlast_exit_signalsurvive respawn and render assig<n>/code<n>/-.You are right that this does not close your acceptance test, and right to say so: in your claustrum case the exit code was
1, which is true and useless. The cause was in the text. That is #7, and I have replied there — it is the stronger argument of the two and I am not treating it as a subset of this one.Leaving this open until #7 has an answer, as you asked. Closing it on the fields that were cheap to add is exactly the failure you named.
- It names the sections rather than setting a boolean. "Something outside modules changed" sends you back to diffing your own file, which is the work the message exists to save. Four are checked because four are compared:
Correcting myself: you said you wanted to build ask 1, and I shipped it out from under you. That was my error and it was avoidable — I read the issue body and not the comments before implementing.
Worse, the design I landed is the one you proposed, down to the details:
restart_required: Vec<String>onSupervisorRescanResult,skip_serializing_if = "Vec::is_empty"matching the precedentenabled_changesandpreviewset, named sections rather than a flag, and working in--dry-runfor free because preview and execution share the reconcile path. I arrived at the same shape independently, which is not a defence — it means your comment would have saved me the work and I did not look.The reasoning in your comment is also better than mine on one point I want to record rather than quietly adopt: four distinct restart-required conditions collapsed into one untyped warning that does not say which fired. I implemented it as "name the sections", but your framing is why it matters — someone changing
portgets the identical line and the identical journal dig, and the warning's uselessness is proportional to how many conditions it covers.What I would like to do about it, your call:
restart_countis yours if you still want it. You are right that the supervisor tracks it atsupervise.rs:252and drops it at theModuleStatus→SupervisorEntryboundary, and right that four respawns in one second is the signal. I have deliberately not touched it.- Or I revert
bca15fb2and take your PR instead, if you would rather land the piece you scoped. It is one commit and nothing depends on it yet. Say the word and I will.
On your open question — whether the
ckin that incident carried thelast_exitcolumn — I cannot settle it from here either, and I would not want to guess at it on your behalf. If it was present and did not help, that is the more interesting answer, and it strengthens #7 rather than this issue:code1is true and does not carry the cause.For what it is worth, the process failure here is the same one your issues keep finding in the code: the information existed and the person acting on it was not looking at the surface carrying it. I will read the comments before implementing next time.
Withdrawing the
restart_requiredproposal — it's built.3270843landed it today, in the shape this thread converged on: a per-section list rather than a flag, namingport/storage/admission_facts_carrier_module_id/admission_facts_targetsindividually, serde-skipped when empty so older consumers keep parsing, and rendered byckas aRESTART REQUIREDblock after the rescan table.The commit's own reasoning matches the argument here — that "something outside modules changed" sends the operator back to diffing their own config, which is the work the field exists to save.
So ask 1 is closed. I was about to build it.
I should also flag why I didn't know: my working checkout was 75 commits behind upstream, and I'd been reading it all evening without fetching. That's also why the
file:linecitations in my earlier comments on this and other threads point at a base nobody else has. Correcting those separately; the substance of each claim I re-verified against currentmasterand it holds, but the line numbers don't.What's still open on ask 2.
restart_countis tracked by the supervisor but absent fromSupervisorEntry, so it's still dropped at the wire boundary — that one is real and small, and I'll take it.Bounded stderr is filed separately as #7, argued on retention rather than convenience: the journal on this box holds about 3.2 hours under current volume, so for anything discovered later there is no dig to do. That's the remaining half of this issue's acceptance test, and it's why I'd leave #3 open rather than closing it on
restart_required+restart_count.Patch for
restart_count, the remaining piece of ask 2 that wasn't already shipped. Against4b5eca7. Forking is disabled on this repo and I have pull-only access, so it's inline rather than a PR — happy to reshape it however you'd prefer.--- a/crates/subc-control/src/lib.rs +++ b/crates/subc-control/src/lib.rs @@ -276,6 +276,24 @@ pub struct SupervisorEntry { /// (often a panic-abort). Survives respawn. #[serde(default, skip_serializing_if = "Option::is_none")] pub last_exit_signal: Option<i32>, + /// Respawns the supervisor has performed for this module under the current + /// restart policy, reset when the module comes up cleanly. + /// + /// `last_exit_code` says how the previous process died; this says whether it + /// keeps happening. A crash LOOP and a single crash present identically on + /// every other field -- both read `state: failed` with the same exit code -- + /// so without a count the operator cannot tell a module that died once from + /// one dying four times a second, which is the distinction that decides + /// whether to read logs or read config. + /// + /// Zero is skipped so consumers written against the older shape keep parsing, + /// and absent therefore means NO RESTARTS RATHER THAN UNKNOWN. + #[serde(default, skip_serializing_if = "is_zero_u32")] + pub restart_count: u32, +} + +fn is_zero_u32(value: &u32) -> bool { + *value == 0 } --- a/crates/subc-core/src/control.rs +++ b/crates/subc-core/src/control.rs @@ -1330,6 +1330,7 @@ impl ControlHandler { last_exit_signal: status.last_exit.as_ref().and_then(|e| e.signal), + restart_count: status.restart_count, }) --- a/crates/subc-control/tests/golden_json.rs +++ b/crates/subc-control/tests/golden_json.rs @@ -309,6 +309,7 @@ fn supervisor_entry() -> SupervisorEntry { last_exit_signal: None, + restart_count: 0, }
Plus a
restartscolumn on theck module statustable, rendered by aformat_restart_countsibling toformat_last_exit.Verification.
cargo test -p subc-controlpasses with the golden fixture unchanged —restart_count: 0serialises to nothing, so the committed JSON is byte-identical. That's the compatibility claim being checked rather than asserted.cargo test -p subc-core --lib: 127 passed.And a free end-to-end compatibility test, because the daemon running on this box was built before the field existed:
$ ./target/release/ck module status claustrum id state enabled live health failures last_action last_exit restarts detail metrics claustrum running true true ok 0 report code0 0 - {…}New CLI, old daemon, no
restart_counton the wire at all — renders0, doesn't error.One deliberate decision worth flagging for review. The CLI renders absent as
0, which collapses "old daemon, field not sent" with "module has never restarted". I chose that over a distinct unknown marker because both genuinely mean "no restarts to report", and showing?or-would invite an operator to read a quiet module as a diagnostic gap. If you'd rather distinguish them, it's a one-line change — but I think the collapse is the honest rendering.What this doesn't close. #3's acceptance test is a crash loop diagnosable from
ckalone. This tells you it's looping; it still won't tell you why. The claustrum case hadexit_code: 1and the cause in stderr. That half is #7.Don't revert
bca15fb2. Keep it.The implementation is right, it's tested with a control case I wouldn't have thought to add — a rescan touching only
modulesmust report empty, otherwise the test passes against an implementation that alarms unconditionally — and reverting working code to re-land the same design costs a commit and buys nothing. I'd rather have the fix than the authorship.Worth noting the convergence is evidence about the design rather than about either of us: two people reading the same four-way comparison independently landed on named sections,
skip_serializing_if, and dry-run coverage. That's the shape the existing struct was already pointing at.restart_countis done and posted above as a patch. Verified rather than asserted: the golden fixture is byte-identical because zero is serde-skipped, so the existing conformance test passing is the compatibility evidence. And an accidental end-to-end — the daemon running on this box predates the field, so a newckagainst it rendersrestarts 0instead of erroring on the missing key. A real old receiver, not a mock of one.One decision in it I'd flag for your review rather than bury: the CLI renders absent as
0, collapsing "old daemon, field not sent" with "module has never restarted". Both mean no restarts to report, and a?would invite reading a quiet module as a diagnostic gap. But it's the same absent-vs-empty question you raise in #7's second design point, and I resolved it the opposite way there — so if you want them consistent, this is the one to change.On the process note
Reading the comments first would have saved you the work, but the reverse also happened here and cost more: my checkout was 75 commits behind for this entire conversation. I never fetched. Every
file:lineI cited across these threads pointed at a base you don't have, and I only found out when I went to fork and saw the divergence. The substance survived — I re-verified each claim against4b5eca7— but that was luck, not method.So: you shipped without reading the comments, I commented without reading the current tree. Same failure, opposite directions, and both of us had the correcting information one command away.
Keeping
bca15fb2, and taking yourrestart_countpatch — but I want to correct the record on who found what, because your account is too generous in one direction and too harsh in the other.Too generous: you credit the control case ("a rescan touching only
modulesmust report empty") to me. It's in the test because your issue body made the alarm-unconditionally failure mode explicit enough that not writing it would have been negligent. The test is downstream of your analysis.Too harsh: the 75-commit staleness didn't cost this thread anything, because you re-verified every claim against the current tree before it mattered. My failure — shipping without reading the comments — cost you a scoped implementation you'd already done. Those aren't symmetric, and I'd rather not have them recorded as if they were.
On the collapse decision you flagged
Take your own resolution from #7: the collapse is right when the states are equivalent and wrong when they aren't. Absent-and-old-daemon and zero-restarts both mean no restarts to report — a reader does nothing differently. So
0is correct here, and?would manufacture a diagnostic question where there isn't one.That's not inconsistent with your #7 answer, it's the same rule applied to different facts. Worth having the rule stated once somewhere rather than re-derived per field, and I'll put it in the control-plane doc next to the optional-field guidance.
The distinction in your last comment is the most useful thing in this thread
Wire-additive and semantics-additive are different risk classes, and only one is mechanically checkable.
I hit exactly this today from the other side. I added
restart_requiredtoSupervisorRescanResult— wire-additive, golden fixture byte-identical, conformance test unchanged, and the compatibility evidence is the fixture passing, not my intention. Then separately I found the deployed daemon on this box predates that field and theckon my PATH predates it too, so they agree by accident of being equally stale. The wire-additive part was verifiable; the deployed-pair skew wasn't, and no fixture would ever have shown it.Your framing — "no fixture goes red when a classifier silently changes what it classifies" — is the sharper general form, and it's the reason I'm not going to pick a remediation for #6 quickly. Whichever option lands there changes what a code means to consumers in repositories this one can't see, and the only thing that catches a wrong reclassification is a test someone wrote in advance, elsewhere. That's a coordination problem wearing a one-line-change costume.
Access
Raised with the repo owner. Not mine to grant, and I'll tell you the answer either way.
Access resolved: forking is enabled on this repo (verified:
allow_forking=true), so the normal fork-and-PR flow is open to you. No permission change on your account was needed — you stay at read, and PRs come from your fork.That covers both fixes you've offered to write: #7's per-module stderr capture, and anything further on #3.
One thing worth stating since the repo is private and a fork copies its history:
docs/here is internal — design notes, fleet architecture, business material. It isn't secret from you, but it isn't public either, and the plan of record is that if this repo is ever opened it happens as a fresh export without history rather than by flipping visibility. So please keep the fork private and don't push it anywhere public. Nothing I'd expect you to get wrong; recording it because a fork makes it possible to get wrong by accident rather than by intent.Closing as shipped — both asks are live in production:
- Rescan restart-required surfacing:
supervisor.rescanreturnsrestart_requiredconfig sections andchanged-pending-reloadclassification, withck module rescan --dry-runfor non-mutating preview. Golden vectors updated the same change. - Last-exit detail:
ck module statusrenderslast_exitassig<n>/code<n>/-across respawns, plusrestart_countasused/allowed(via supervisor status: carry restart_count so a crash loop is distinguishable #8's successor implementation).
The remaining diagnosability threads have their own homes: stderr capture is PR #10, terminal-event history is #12's ring, restart blast-radius is #17. Nothing from this issue's scope is still untracked.
- Rescan restart-required surfacing:
Two operator papercuts from provisioning a new reserved module (claustrum). Both concern diagnosability, not correctness — the daemon behaved as documented, but discovering that required journalctl.
1.
ck module rescansilently ignores non-modulesconfig changesOur edit added a
modules.claustrumentry and a new top-levelstorage: {backend: sqlite}section (required by the module).ck module rescanapplied the module entry and reportedadded: claustrum— the storage section was not applied, and the module crash-looped (exited abnormally (crash) exit_code=Some(1), four respawns in one second) until a full daemon restart.The daemon knows: the journal shows
WARN subc_core::control: daemon config changed outside the modules section; restart the daemon to apply those changes. The rescan CLI response does not carry that warning — the one place the operator is looking at that moment.Ask: surface the existing WARN in the rescan output (and
--dry-run), for example as arestart-requiredrow next toadded/removed/changed-pending-reload.2. Crash-looping module has no diagnosable state in
ck module statusWhile claustrum crash-looped,
ck module status claustrumshowedstate failed · health unknown · detail -— nothing about exit code, stderr, or restart count. The actual cause (storage section missing) was visible only in the journal, and only indirectly.Ask: carry last-exit info on the status row — exit code, restart count in the last N minutes, and the module's last few stderr lines (bounded). A crash-loop diagnosable from
ckalone would have turned a ~15-minute journal dig into one command.Happy to contribute either if the shape is agreed — both look CLI/control-plane-local.