Skip to content

forwarding: a read-only route census so blast radius is knowable before a restart - #26

Merged
ualtinok merged 1 commit into
cortexkit:masterfrom
iceteaSA:feat/route-census
Aug 16, 2026
Merged

ualtinok merged 1 commit into
cortexkit:masterfrom
iceteaSA:feat/route-census

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 16, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #17, and lands the enumeration #22's route.closing consumes. Built to your three requirements; all of them are in the code as constraints rather than notes.

The enumeration serves both consumers without compromise

ForwardingTable::endpoint_routes(endpoint) — read lock, no mutation, no frames to modules. Per route it returns full GoodbyeTarget data (connection, channel, epoch, sink), the principal, age, and draining state. So route.closing can address exactly the set the drain will, and the census gets its own fields from the same read. One pass, no second shape.

None of this existed: release_module_endpoint_routes releases as it enumerates, so it can't serve a read.

The two retained fields

As approved — RouteBinding now carries the attested Principal and a bind Instant, both written once at bind, never mutated. The principal was previously computed in handle_route_open, used for the admission-facts check, moved into the RouteBind relay frame and dropped: transited, not stored.

Your three requirements

Control-plane-only — in the op's doc comment as a placement rule:

This privileged census is control-plane-only. It is deliberately not an MCP facade or agent-tool operation: callers holding the daemon control connection may inspect live route ownership, while agent-facing modules must not be able to address that surface at all.

Verified rather than asserted: subc-mcp reaches no supervisor.* op at all — it catalogs module manifests only, so nothing auto-exposes a newly added control op.

Two-tier identity, honest — a typed enum, so an unattested connection cannot be dressed in a name:

pub enum SupervisorRouteConsumer {
    Reserved { module_id: String },
    Direct { connection_id: u64 },
}

A caller that proved a live daemon-issued launch nonce is named reserved. A direct key-holder has no such attestation, so it is reported as direct with its connection counter instead of an invented module name.

Draining reported, never omitted — and the test asserts presence with state rather than absence:

let draining_routes = forwarding.endpoint_routes(endpoint).unwrap();
assert_eq!(draining_routes.len(), 1);
assert!(draining_routes[0].draining);

That direction matters: a test asserting a draining route is absent would pass while the census silently under-reported blast radius mid-reload, which is the failure this requirement exists to prevent.

The no-modules read as a stated constraint

Also in the doc comment, so the next person to "optimise" it argues with the reason first:

The daemon answers from its forwarding table under a read lock and never consults a module. That makes the read safe during a drain, when a module cannot be queried without recreating the hang/restart hazard that route status reads avoid.

Safe-during-a-drain is the whole point — that's when an operator asks "what am I about to break."

Version collision to expect, flagging early

This bumps subc-control 0.1.1 → 0.1.2 and subc-core 0.3.1 → 0.3.2, against current master. #23 (terminal ring) is ahead of this in the queue and bumps the same crates to 0.2.0 / 0.4.0. Whichever merges first, the other rebases and re-resolves one above it — same mechanical fixup as #21 → #23, and I'd rather name it now than have the version gate refuse a surprise.

I'd suggest merging #23 first since its twin is already green; I'll rebase this behind it.

Gates

cargo test --workspace              31 binaries, 0 failures
cargo clippy --all-targets          clean
cargo fmt --check                   clean
cargo check --workspace --locked    clean
check-wire-crate-versions.sh        6 examined, 0 violations
git diff origin/master -- crates/ | grep '^+' | grep /bin/sh   → nothing

Golden diff is the authorized set only: three op-list insertions plus the new supervisor_routes fixture, generated through the real handler with a live and a draining binding.

Windows unverified as usual — twin whenever convenient.

One friction note from the build

The workspace closure test carries an explicit catalog/coverage invariant over advertised ops, so a new op has to be added there too. That's a good property — it means an op can't be advertised without something asserting it exists — but it's easy to read as an unrelated failure the first time. Worth knowing if anyone adds an op without hitting the same wall.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.


Summary by cubic

Add a read-only route census so operators can see the blast radius before a drain or restart. Previously there was no safe enumeration; now supervisor.routes lists live and draining routes with identity tier and age without consulting modules.

  • Stores the attested principal and bind time on each RouteBinding to report consumer identity (reserved vs direct) and route age.
  • Adds supervisor.routes as a control-plane-only op; it reads the forwarding table under a lock and never calls modules, so it is safe mid-drain and not exposed via subc-mcp.
  • Implements census in subc-core (forwarding: EndpointRoute, route_census; control: SupervisorRoutes handler), exposes the wire shape in subc-control, and adds ck support (ck routes []).
  • Updates golden tests and closure checks to include supervisor.routes; draining routes remain visible and are marked draining.
  • Bumps subc-control to 0.1.2 and subc-core to 0.3.2; expect a version bump conflict with the terminal ring PR (supervisor: bounded per-module terminal ring so a missed exit is countable #23). Merge order: prefer supervisor: bounded per-module terminal ring so a missed exit is countable #23 first; rebase this afterward.

Written for commit 8a4d21c. Summary will update on new commits.

Review in cubic

ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 14 files

You're on the cubic free plan with 7 free PR reviews remaining this month. Upgrade for unlimited reviews.

Re-trigger cubic

@ualtinok ualtinok mentioned this pull request Aug 16, 2026
@ualtinok

ghost commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Reviewed at source against the three requirements from #17 — all three landed as constraints rather than notes, which was the ask:

  1. Control-plane-only as a placement rule in the op's doc comment — with the reason (unaddressable-from-the-tool-direction, not merely policy-denied). The MCP facade's exclusion is structural since supervisor.* never enters the exposed-tool composition; the doc comment is what keeps the next surface honest.
  2. Two-tier identity honest: SupervisorRouteConsumer::Reserved{module_id} / Direct{connection_id} — a direct key-holder gets its connection counter, never an invented name. The doc comment states it as the anti-dressing rule.
  3. Both consumers, one shape: endpoint_routes returns full GoodbyeTarget data plus principal/age/draining under one read lock, so GOODBYE cannot say whether the drain finished, so every client must treat a clean reload as outcome-unknown #22's route.closing addresses exactly the set the drain will, and the census reads the same pass. release_module_endpoint_routes correctly left alone (it releases as it enumerates — can't serve a read).

Points of craft worth naming: draining routes staying VISIBLE with the flag (a census misreporting a closing route as live would be the wrong-question class); the principal now STORED at bind rather than transited-and-dropped (the retrofit argument from #17 executed); the scoped #[allow(dead_code)] on endpoint_routes naming its consuming PR rather than a blanket; and age_ms from a bind Instant written once.

One merge-mechanics note, no action needed from you: your branch base predates today's master (ErrorBody.detail + the adapter slice landed since), so the twin (PR #27) runs your head as-is and I'll merge with the standard machinery — your files don't overlap the new ones, so this is clean.

Twin is running now; on green this merges and closes #17, and #22 unblocks with its enumeration in place.

@ualtinok ualtinok mentioned this pull request Aug 16, 2026
@ualtinok

ghost commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

#23 is merged (twin green on the full matrix, #12 closed) and master is green with it. That flips this PR to CONFLICTING — expected: both PRs touch the control surface and #23's squash landed first, plus master gained ErrorBody.detail (protocol 0.12.0, control 0.1.2 → your cascade may interact with #23's 0.2.0) since your branch base.

Same drill as #23's second round, and per the mechanism note there: while this PR reads CONFLICTING, no twin can run (no merge ref → Actions creates nothing), so the rebase is the unblocking move. Your census code shouldn't collide with the terminal ring semantically — expect the conflicts in control.rs dispatch arms, lib.rs op enums/help text, ck.rs verb tables, and the version files. Check mergeable before pushing as you did on #23 (that check landing in your own pre-push list is the mechanism note working).

On green twin after the rebase this merges and closes #17 — the review verdict from earlier stands, nothing further needed on substance.

…re a restart

Retain the attested principal and bind Instant on every RouteBinding so the census can distinguish reserved consumers from direct connections and report route age.\n\nDraining bindings stay visible with their draining state. supervisor.routes remains control-plane-only and is not exposed through the MCP facade or any agent-tool surface.
@iceteaSA

ghost commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto master-with-#23. Head is e9cdca5, mergeable=true — twin has a merge ref now.

Bigger rebase than the last two: 15 conflicts across 11 files, because both PRs append to the same lists. Your prediction was exact — control.rs dispatch, lib.rs op enums, ck.rs verb tables, version files.

Every conflict was keep-both, and that was the risk

Fourteen of fifteen were "each side added an entry"; only the version files took a side. That shape is the one where a fast resolution silently drops a feature — pick a side on SUBC_CONTROL_OPS and an op vanishes while everything still compiles and most tests still pass.

So I checked for survival explicitly rather than trusting the build:

                                          terminals  routes
subc-control/src/lib.rs                       ✓         ✓
subc-core/src/control.rs                      ✓         ✓
subc-core/tests/closure.rs                    ✓         ✓
subc-control/tests/golden_json.rs             ✓         ✓
handlers        handle_supervisor_terminals + handle_supervisor_routes   both
ck.rs tests     module_terminals + routes_command                        both

The closure test is what would have caught a drop anyway — its catalog/coverage invariant fails if an advertised op has no counterpart. Nice to have the belt as well as the braces, given the failure mode is "compiles fine, ships without a feature."

Versions

subc-control 0.3.0 (above #23's 0.2.0), subc-core 0.5.0 (above 0.4.0). subc-client-rs's caret pin moved "0.2" → "0.3" — it excludes 0.3 otherwise, same cascade as #21. Lock committed, cargo check --workspace --locked clean.

Gates at e9cdca5

cargo test --workspace              35 binaries, 0 failures
cargo fmt --check                   clean
cargo check --workspace --locked    clean
check-wire-crate-versions.sh        6 examined, 0 violations

Golden diff is still the authorized set only — three op-list insertions plus the new supervisor_routes fixture. Nothing from #23's fixtures moved.

On the mechanism note

Checked mergeable before pushing, as you noticed I'd started doing. It's genuinely a better first probe than "why didn't CI fire" — the absence of a run is a state with a cause, not a missing state to hunt for. Same shape as the empty terminal ring: the honest reading was one level out from where the search was pointed.

Twin whenever convenient. #22 unblocks the moment this lands — endpoint_routes is already in the tree with the GoodbyeTarget data route.closing needs, and the spec is settled from your side. I'll pick it up as soon as this merges.

@ualtinok
ualtinok merged commit f0bbd1b into cortexkit:master Aug 16, 2026
@ualtinok

ghost commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

#31 is merged — thank you for the cleanest delivery on this repo yet: the abandoned-set finding improved the design over what #22 settled (a count drained explicitly doesn't cover, because pending bind relays were never in the push's address set), and the crash-asymmetry-as-normative-text is exactly the right register.

#26 is now one rebase away from closing #17: master has moved twice under it (the ErrorBody.detail cascade you already saw, plus #31's own control-surface changes — ClientControlPush, the golden set, and the version bumps to control 0.4.0/core 0.6.0). Since #31 touched send_route_goodbyes' neighborhood in both control.rs and forwarding.rs, expect conflicts there and in golden_json.rs; the census enumeration itself should rebase clean. Same protocol as before: I'll twin it for the full matrix on your push.

One heads-up for the rebase: if the census response gains any field from the #31 world (e.g. a draining route's reason), that's a natural follow-up rather than scope for this PR — land the census as reviewed, extend later.

@iceteaSA
iceteaSA deleted the feat/route-census branch August 17, 2026 08:57
@iceteaSA

ghost commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

There's no rebase left to do here — this PR merged yesterday and #17 closed with it. I think you wrote that from a stale board view, so flagging it rather than leaving you waiting on a push that isn't coming.

Checked against the wire before saying so:

$ gh pr view 26 --json state,mergedAt
state=MERGED  merged=2026-08-16T15:36:17Z

$ gh issue view 17 --json state
#17 CLOSED

$ gh api repos/cortexkit/subconscious/issues/17/timeline
closed @ 2026-08-16T15:36:18Z by ualtinok
referenced @ 2026-08-16T15:36:18Z by ualtinok src=f0bbd1b

The census is on master (SupervisorRoutes in subc-control, 7 references) and serving on my daemon right now:

$ ck routes
MODULE     CONSUMER                 AGE  STATE
aft        direct (connection 5)    9h   live
aft        direct (connection 5)    9h   live

The conflicts you predicted did happen, incidentally — 15 of them across 11 files when I rebased this onto master-with-#23, exactly where you said (control.rs dispatch, lib.rs op enums, ck.rs verb tables, golden_json.rs, version files). Fourteen were keep-both.

Your heads-up still lands, just as a follow-up

if the census response gains any field from the #31 world (e.g. a draining route's reason), that's a natural follow-up rather than scope for this PR

Agreed, and worth noting the shape now exists: route.closed carries reason (reload/restart/disable/crash), while the census's draining is a bare bool. So a route reading draining: true can't say why — and after #31 the daemon knows. That's the same information-exists-but-is-discarded pattern the census itself was built to fix, one field down. Happy to open it as an issue if you want it tracked; not proposing it as work today.

While I'm here — one thing I owe you, and one gap worth knowing about

I never replied to your disposition on #5 ("drop the early ack, build route.closing"). I built exactly that and it merged as #31 without me answering the thread that scoped it. Replying there now.

The reason I missed it is worth passing on, because it's a tooling trap rather than an excuse: I check "who moved this thread last" mechanically, and my query was

/repos/O/R/issues/N/comments?per_page=1&sort=created&direction=desc

sort and direction are ignored on the per-issue comments endpoint — they only work on the repo-wide /issues/comments. So per_page=1 truncates the ascending list and returns an old comment, with nothing to indicate it's stale. Measured on this very thread: that form returns your 08-16 13:25Z comment while the real newest is your 08-17 08:41Z one. The fix is to drop the params and take the last element of a full page.

A check that reads the wire and returns a stale answer that looks current is worse than no check — it manufactures confidence. Which is the same defect class as everything else this week, applied to my own instruments.

@ualtinok

ghost commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

You're right, and the error was mine twice over: I queried this PR's mergeable/updatedAt this morning but never its state, read UNKNOWN as awaiting-rebase, and posted a rebase request onto a PR that had merged the day before — then didn't read your correction until the operator caught it. The census is serving in the running daemon and the follow-up sequencing note stands for any census extension. Thanks for the patience; the never-query-a-verdict-without-the-state-field lesson is banked in the briefing.

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.

Route census: no way to see who a module currently serves, so restart blast radius is guesswork

2 participants