Skip to content

ck, daemon: say when a module is running something other than what its config asks for (#108) - #120

Closed
iceteaSA wants to merge 1 commit into
cortexkit:masterfrom
iceteaSA:feat/pending-reload-verdict
Closed

iceteaSA wants to merge 1 commit into
cortexkit:masterfrom
iceteaSA:feat/pending-reload-verdict

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Linked issue

Approved issue: #108

What changes

  • Rescan preview no longer says "would restart". It says would change (pending reload), the tense-shift of the apply label changed-pending-reload. The unreachable "restarted" arm is deleted. After an apply that deferred anything, ck prints one line saying the restart is deferred and names ck module restart <id>.
  • SupervisorEntry.pending_reload: Option<PendingReloadVerdict> (subc-control 0.20.0, additive, serde default). The daemon derives it in supervisor.list from two facts it already has, and stores nothing new:
    • path: SupervisorObservedProcess.spawned_from vs the currently configured program. Differing means the config asks for a different program than the one running.
    • image: the existing RunningImageAgreement. Mismatch means the file under this program changed since spawn (a replace at a stable path).
  • Three-valued throughout. A stopped module, or any Unavailable { reason } (not running, unreadable path, unconfirmed identity, unsupported platform), is rendered as cannot tell, with the reason. It is never "nothing pending" and never "pending". A None from an older daemon also renders as unknown. ck module list gets a compact marker (pending (path), pending (image), pending (path+image), unknown, nothing), and ck module status explains each signal separately: they call for different actions (fix the config or respawn, versus investigate a swapped file).
  • Forward compatibility. The path enum has an Unknown decode fallback, and the image side reuses RunningImageAgreement's existing one, so an older client doesn't fail the whole supervisor.list decode on a newer variant. Every rendered daemon- or module-supplied string goes through ck's terminal-safe escaping.

Cost

It reuses ExecutableIdentityProbe and its digest cache, with no new hashing path. Warm calls re-stat only. A cold list (the first after a daemon restart or a binary swap) hashes each module image serially, because the probe holds its digest-cache mutex across hashing (provenance.rs:91-95). supervisor.provenance already pays the same cost. I left provenance.rs untouched on purpose: it's the PID-recycling / TOCTOU path from #60/#76, and changing its lock scope belongs in its own change. No supervisor, registry or state lock is held across the probe await: operands are snapshotted, the guard is released, then the probe runs.

Windows (as ruled)

Windows has no running-image probe, so the image signal there is always Unavailable{unsupported_platform}. Per your ruling, the list marker separates a probe that doesn't exist on this platform from one that failed this time:

  • path differs, on any platform: pending (path), whatever the image says;
  • path matches and the reason is exactly unsupported_platform: nothing (image n/a); ck module status says running image: not checked on this platform (unsupported_platform);
  • path matches and the image probe exists but couldn't answer (any other reason, including a future one): unknown;
  • path itself unavailable: unknown.

Only the exact reason string unsupported_platform gets the n/a rendering; every other reason, known or future, stays unknown. ck_cli tests split on the probe's own cfg gate with a positive assertion per arm. A new test, module_list_keeps_configured_path_mismatch_pending_on_every_platform, changes a module's configured program through the existing update_spec_for_test fixture and asserts pending (path) on every platform; on the Windows arm it also asserts that the image reason is unsupported_platform, so path precedence is proven there. Mutation proofs: rendering unsupported_platform as unknown reddens three named unit tests; rendering every unavailable reason as n/a reddens the other-reason and future-reason tests; inverting the capability condition reddens the three platform snapshots.

What it doesn't close

The window is narrowed, not closed: a module can still be stale for reasons neither signal sees (a shared library swapped under an unchanged binary, for example).

Verification

  • Tests on the pure derivation and on the renderer, each shown red by breaking its seam:
    • path mismatch;
    • image mismatch;
    • each Unavailable reason stays unknown under collapse in both directions (to "nothing" and to "pending"), including a future reason;
    • path pending while the image is unavailable, so neither side hides the other;
    • an old daemon's response decodes to None, which renders as unknown;
    • the preview label pairs with the apply label and has no "restart".
  • New golden supervisor_entry_reload_verdict_states.json covers every state. No existing golden changed.
  • Cross-family review (grok-4.5): APPROVE after one fix round. It found that the unsupported-platform collapse wasn't mutation-locked; its two mutations now go red.
  • At 55d32f15 (rebased on d3a35346): fmt ✓ · clippy --workspace --all-targets --locked -D warnings ✓ · check-wire-crate-versions.sh ✓ (13 crates) · ck_cli 55/0 · ck binary 246/0. The full workspace suite wasn't re-run locally after this rebase; your CI runs it on this push.
  • At the previous head 00af6f41 (same daemon and wire code, rebased on 2f1c884f): full workspace tests with 2 failed targets, both ck-bus NATS tests that need a nats-server binary this host doesn't have · subc-daemon lib 400/0 · TS client 167/0.
  • Rebase note: master independently took subc-control 0.19.0, the number this branch had chosen, and git merged the identical line without a conflict, so the bump would have vanished silently. Versions are set above master rather than to either side's: control 0.20.0, daemon 0.21.4, core 0.20.23, client-rs 0.18.5, ck-bus 0.1.11, with the three versioned subc-control requirements moved to 0.20. The one code conflict, in ck.rs, was both sides appending tests at the same place; I kept both.

Unrelated, measured while checking a red I saw mid-build: control::tests::route_open_refusal_names_the_check_that_refused and three fleet_lint tests fail on master under CPU load: 1 run in 25 of the daemon lib binary with 8 busy loops running alongside, 0 in 25 without. They pass alone, and on this branch 0 in 25. Not from this change; noted in case your CI hits them.

CONSUMER-IMPACT: subc-control 0.20 adds SupervisorEntry.pending_reload (serde default; old daemons decode as None). No behaviour change to rescan itself. You said you'd send the fleet notice at merge.

@cortexkit-ci

cortexkit-ci Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

#108 has the design-approved label; this PR can be reviewed.

@subc-alfonso

subc-alfonso Bot commented Sep 24, 2026

Copy link
Copy Markdown

Thanks. The capability-split tests and the Approved issue: link are exactly right, and every platform leg is green. The design gate will pass once the label is applied on #108.

On Windows: yes, show the path half, as you suggest. The three-valued rule is about not claiming what the daemon couldn't check. A signal that is permanently unavailable on a platform is different from one that failed this time, and the marker can say so without claiming more than it knows:

  • path differs, on any platform: pending (path), whatever the image signal says;
  • path matches and the image probe doesn't exist on this platform: nothing (image n/a);
  • path matches and the image probe exists but couldn't answer this time (a process identity it couldn't confirm, an unreadable file): unknown, as now.

So unknown keeps meaning "we tried and couldn't tell", and a Windows host still gets a per-module answer for the half it can check. Please make ck module status say the same thing in words, and add one test on the Windows arm with a differing path, so pending (path) is proven there too.

@iceteaSA
iceteaSA force-pushed the feat/pending-reload-verdict branch from 00af6f4 to 55d32f1 Compare September 24, 2026 11:23
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Done as ruled, at 55d32f15, rebased on d3a35346.

  • The list marker renders nothing (image n/a) only when the path matches and the image reason is exactly unsupported_platform. A path mismatch still wins as pending (path), and every other image reason, known or future, stays unknown. ck module status now says running image: not checked on this platform (unsupported_platform).
  • The test you asked for is module_list_keeps_configured_path_mismatch_pending_on_every_platform. It changes the configured program through the existing update_spec_for_test fixture and asserts pending (path) on every platform. On the Windows arm it also asserts that the image reason is unsupported_platform, so precedence over the image is proven there, not assumed.
  • Mutations: rendering unsupported_platform as unknown reddens three named unit tests; rendering every unavailable reason as n/a reddens the other-reason and future-reason tests; inverting the capability condition reddens the three platform snapshots.

The PR description's Windows section and verification lines are updated to match, including which gates ran at this head. Versions are unchanged from the rebase (daemon 0.21.4, ck-bus 0.1.11, above master).

@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Rebased onto 29a6375a after #127 merged. #127 took subc-client-rs 0.18.5, the number this branch also had, and git merged the identical line without a conflict. So I moved this branch to 0.18.6 by hand. Other versions are unchanged: control 0.20.0, daemon 0.21.4, core 0.20.23, ck-bus 0.1.11.

The code patch is identical to 55d32f15, excluding manifests and the lockfile. At 734c3eda: fmt ✓ · clippy --workspace --all-targets --locked -D warnings ✓ · check-wire-crate-versions.sh origin/master ✓ (13 crates) · ck_cli 55/0 · subc-client-rs + subc-control 125/0.

@subc-alfonso

subc-alfonso Bot commented Sep 24, 2026

Copy link
Copy Markdown

Thanks for the rebase. Two version numbers still need to move before I can merge:

  • subc-client-rs 0.19.0, not 0.18.6. This PR moves its subc-control requirement from 0.19 to 0.20, and subc-client-rs re-exports subc-control types (CatalogEntry, ConsumerIdentity). A consumer holding those types from 0.18.5 would get type mismatches after a patch update, so this is a minor bump.
  • ck-bus 0.1.13. A ck-bus slice landed on master at 0.1.11 after your rebase (a64fa0d), and the next one is about to take 0.1.12, so the same number would name two different crates.

With those two changes bash scripts/fleet/check-wire-crate-versions.sh origin/master should pass, and I'll run the full gate and merge. One heads-up: a daemon fix of mine (#125) is about to land with subc-daemon 0.21.4 and subc-core 0.20.23 too. Whichever merges second takes the next patch number; if it's yours, I'll tell you.

…s config asks for (cortexkit#108)

Derive independent path and running-image signals from the current launch spec and the existing executable identity probe. Unknown stays distinct from agreement, including stopped processes and responses from older daemons. Relabel the rescan preview to match deferred apply; apply still stores the spec without restarting.

A cold supervisor.list hashes each module image serially under the probe’s digest-cache lock (same cost as supervisor.provenance); warm calls re-stat only.

CONSUMER-IMPACT: subc-control 0.20 adds `SupervisorEntry.pending_reload` (serde default; old daemons decode as None). No behaviour change to rescan.
@iceteaSA
iceteaSA force-pushed the feat/pending-reload-verdict branch from 734c3ed to 88f73f5 Compare September 24, 2026 13:33
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Both done at 88f73f5b, rebased on current master (includes a64fa0d):

  • subc-client-rs 0.19.0, and ck-bus's requirement on it moved from 0.18 to 0.19, since it re-exports the subc-control types. mcp-stdio-adapter depends on it by path only, so there was nothing to move there.
  • ck-bus 0.1.13.

The Cargo.lock entries, read by name, show 0.19.0 and 0.1.13. The code patch is identical to 734c3eda, excluding manifests and the lockfile. check-wire-crate-versions.sh origin/master passes (13 crates) · fmt ✓ · clippy --workspace --all-targets --locked -D warnings ✓ · ck_cli 55/0 · subc-client-rs + subc-control + ck-bus 68 passed, 1 failed: ck-bus a1_supervised_server_lifecycle, "real nats-server binary required on unix", and this host has no nats-server.

Noted on #125. If it lands first, I'll move daemon/core to the next patch numbers.

@subc-alfonso

subc-alfonso Bot commented Sep 24, 2026

Copy link
Copy Markdown

Thanks, both versions are right now. Checked on your head rebased onto current master (cba2285):

  • cargo fmt --check, and clippy with -D warnings for the whole workspace on macOS and x86_64-pc-windows-gnu: clean
  • cargo check --target x86_64-unknown-linux-gnu -p subc-daemon --all-targets: clean
  • cargo test --workspace --locked: 1761 passed, 0 failed
  • bun test in clients/subc-client: 175 pass, 0 fail
  • version check against master: passes (subc-control 0.20.0, subc-client-rs 0.19.0, ck-bus 0.1.13)

Master moved again after your push (subc-client-rs 0.18.6 and 0.18.7, and a ck-bus slice), so the rebase conflicted only on the manifest lines and the lockfile. I resolved them to your numbers, keeping master's subc-protocol floor of 0.25.2 in subc-client-rs. The branch isn't open to maintainer edits, so I pushed the rebased commit to master as 8050603, with you as its author, and I'm closing this PR as merged there. Thanks for seeing #108 through.

@subc-alfonso subc-alfonso Bot closed this Sep 24, 2026
@iceteaSA
iceteaSA deleted the feat/pending-reload-verdict branch September 24, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant