From 57fa5d66ae3cfe10e08589024d9939dec38aa9b4 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:09:39 +0000 Subject: [PATCH 1/9] =?UTF-8?q?feat(cp):=20observer=20scaffold=20=E2=80=94?= =?UTF-8?q?=20observer=20agent=20type,=20cp/event=20+=20cp/list=5Fagents?= =?UTF-8?q?=20wire=20types=20(Phase=201=20scaffold)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scaffold for the CP observer/lobby roadmap (Phase 1 of 3): - AgentType::Observer — read-only lobby client identity - cp/event notification envelope (JsonRpcNotification, EventParams with monotonic seq) and CpEvent payloads (agent_registered/deregistered, delegation_requested/completed/cancelled) - cp/list_agents wire types (AgentSummary, ListAgentsResult) - registry: observers excluded from select(), observers() fan-out set - policy: ObserverInitiation denial — unconditional, no relaxation knob - config: max_event_excerpt_bytes cap for event-mirrored payloads - cp.toml.example: observer identity example Server-side wiring (registration path, event emission, list_agents handler) lands in the Phase 1 implementation commits. --- crates/openab-cp/cp.toml.example | 13 +++ crates/openab-cp/src/config.rs | 9 ++ crates/openab-cp/src/policy.rs | 47 ++++++++- crates/openab-cp/src/proto.rs | 167 +++++++++++++++++++++++++++++++ crates/openab-cp/src/registry.rs | 33 +++++- 5 files changed, 265 insertions(+), 4 deletions(-) diff --git a/crates/openab-cp/cp.toml.example b/crates/openab-cp/cp.toml.example index 9ae1c1642..7b15c959c 100644 --- a/crates/openab-cp/cp.toml.example +++ b/crates/openab-cp/cp.toml.example @@ -74,6 +74,10 @@ max_inflight_delegations = 4096 # enormous budget would otherwise disable saturation entirely. default_max_delegated_sessions_cap = 16 +# Prompt/result excerpts mirrored to observers in cp/event frames are +# truncated to this size (the lobby is an audit surface, not a payload path). +max_event_excerpt_bytes = 4096 + [[agents]] key = "${CP_KEY_KOUDU}" # per-agent secret, never shared namespace = "prod" @@ -87,6 +91,15 @@ name = "worker-1" type = "worker" max_delegated_sessions_cap = 4 # overrides default_max_delegated_sessions_cap +# Read-only lobby client (e.g. the macOS/iOS lobby app). Observers receive +# cp/event notifications and may call cp/list_agents; they can never +# initiate, serve, or cancel delegations — no config can relax this. +[[agents]] +key = "${CP_KEY_LOBBY}" +namespace = "prod" +name = "lobby-app" +type = "observer" + # Per-namespace policy. Absent namespaces use the conservative defaults: # max_depth = 1, allow_worker_initiation = false. [namespaces.prod] diff --git a/crates/openab-cp/src/config.rs b/crates/openab-cp/src/config.rs index 4385eaeb6..ce81d3eb8 100644 --- a/crates/openab-cp/src/config.rs +++ b/crates/openab-cp/src/config.rs @@ -90,6 +90,11 @@ pub struct CpConfig { /// short enough that a dead one frees its quota promptly. #[serde(default = "default_write_timeout_secs")] pub write_timeout_secs: u64, + /// Maximum size of prompt/result excerpts mirrored to observers in + /// `cp/event` frames. The lobby is an audit surface, not a second + /// delivery path: excerpts are truncated with a marker, never rejected. + #[serde(default = "default_max_event_excerpt_bytes")] + pub max_event_excerpt_bytes: usize, /// Memory ceiling for ONE connection's outbound queue, in bytes. /// @@ -180,6 +185,10 @@ fn default_max_delegated_sessions_cap() -> u32 { 16 } +fn default_max_event_excerpt_bytes() -> usize { + 4 * 1024 +} + /// Immutable identity claims bound to one auth key. #[derive(Debug, Clone, Deserialize)] pub struct AgentIdentity { diff --git a/crates/openab-cp/src/policy.rs b/crates/openab-cp/src/policy.rs index 6792de59b..cca9dcc48 100644 --- a/crates/openab-cp/src/policy.rs +++ b/crates/openab-cp/src/policy.rs @@ -32,11 +32,20 @@ pub struct PolicyInput<'a> { #[derive(Debug, PartialEq, Eq)] pub enum PolicyDenial { WorkerInitiation, - DepthExceeded { max: u32, would_be: u32 }, - Cycle { target: String }, + /// Observers are read-only: no config can relax this. + ObserverInitiation, + DepthExceeded { + max: u32, + would_be: u32, + }, + Cycle { + target: String, + }, CrossNamespace, DeadlinePast, - DeadlineTooLong { max_secs: u64 }, + DeadlineTooLong { + max_secs: u64, + }, DeadlineExceedsParent, } @@ -46,6 +55,12 @@ impl std::fmt::Display for PolicyDenial { PolicyDenial::WorkerInitiation => { write!(f, "workers may not initiate delegations in this namespace") } + PolicyDenial::ObserverInitiation => { + write!( + f, + "observers are read-only and may never initiate delegations" + ) + } PolicyDenial::DepthExceeded { max, would_be } => write!( f, "delegation depth {would_be} exceeds namespace max_depth {max}" @@ -70,6 +85,11 @@ impl std::fmt::Display for PolicyDenial { /// Evaluate the full CP-side policy for one delegation attempt. pub fn check(input: &PolicyInput<'_>, ns_policy: &NamespacePolicy) -> Result<(), PolicyDenial> { // 1. Initiator role. + if *input.from_type == AgentType::Observer { + // Unconditional: observers are read-only regardless of namespace + // config; there is no relaxation knob by design. + return Err(PolicyDenial::ObserverInitiation); + } if *input.from_type == AgentType::Worker && !ns_policy.allow_worker_initiation { return Err(PolicyDenial::WorkerInitiation); } @@ -162,6 +182,27 @@ mod tests { assert!(check(&input, &relaxed).is_ok()); } + #[test] + fn observer_initiation_always_denied() { + let now = Utc::now(); + let chain: Vec = vec![]; + let mut input = base(now, &chain); + input.from_type = &AgentType::Observer; + assert_eq!( + check(&input, &default_policy()), + Err(PolicyDenial::ObserverInitiation) + ); + // No namespace relaxation can grant it. + let relaxed = NamespacePolicy { + max_depth: 99, + allow_worker_initiation: true, + }; + assert_eq!( + check(&input, &relaxed), + Err(PolicyDenial::ObserverInitiation) + ); + } + #[test] fn depth_exceeded_at_default_depth_one() { let now = Utc::now(); diff --git a/crates/openab-cp/src/proto.rs b/crates/openab-cp/src/proto.rs index e8ec2bc37..c813ac590 100644 --- a/crates/openab-cp/src/proto.rs +++ b/crates/openab-cp/src/proto.rs @@ -188,6 +188,10 @@ pub mod codes { pub enum AgentType { Primary, Worker, + /// Read-only lobby client (Phase 1 of the observer/lobby roadmap): it + /// receives `cp/event` notifications and may call `cp/list_agents`, but + /// can never initiate, serve, cancel, or complete delegations. + Observer, } impl std::fmt::Display for AgentType { @@ -195,6 +199,7 @@ impl std::fmt::Display for AgentType { match self { AgentType::Primary => write!(f, "primary"), AgentType::Worker => write!(f, "worker"), + AgentType::Observer => write!(f, "observer"), } } } @@ -444,6 +449,113 @@ pub struct CancelParams { pub reason: String, } +// --- cp/event (CP → observer, JSON-RPC notification) --- + +/// JSON-RPC 2.0 **notification** (no id): observers never reply to events. +#[derive(Debug, Serialize)] +pub struct JsonRpcNotification { + pub jsonrpc: &'static str, + pub method: String, + pub params: Value, +} + +impl JsonRpcNotification { + pub fn new(method: impl Into, params: Value) -> Self { + Self { + jsonrpc: "2.0", + method: method.into(), + params, + } + } +} + +/// Envelope of one `cp/event` notification. +/// +/// `seq` is a CP-process-monotonic sequence number: observers detect gaps +/// (dropped frames on a saturated queue, CP restart) by discontinuity and +/// resynchronize via `cp/list_agents`. It is not durable across CP restarts. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct EventParams { + pub seq: u64, + pub ts: chrono::DateTime, + /// Namespace this event is scoped to (matches the observer's own). + pub namespace: String, + #[serde(flatten)] + pub event: CpEvent, +} + +/// Lobby-visible control-plane events. Prompt/result bodies are carried as +/// bounded excerpts (`max_event_excerpt_bytes`): the lobby is an audit +/// surface, not a second delivery path for full payloads. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(tag = "event", rename_all = "snake_case")] +pub enum CpEvent { + AgentRegistered { + /// Logical id, `namespace/name`. + agent: String, + #[serde(rename = "type")] + agent_type: AgentType, + instance_id: String, + labels: std::collections::BTreeMap, + }, + AgentDeregistered { + agent: String, + instance_id: String, + /// `"disconnect"` or `"lease_expired"`. + reason: String, + }, + DelegationRequested { + delegation_id: String, + from: String, + to: String, + prompt_excerpt: String, + deadline: chrono::DateTime, + chain: Vec, + }, + DelegationCompleted { + delegation_id: String, + from: String, + to: String, + status: DelegationStatus, + #[serde(skip_serializing_if = "Option::is_none")] + result_excerpt: Option, + #[serde(skip_serializing_if = "Option::is_none")] + error: Option, + }, + DelegationCancelled { + delegation_id: String, + /// Who cancelled: the initiator's logical id, or `"control-plane"` + /// for deadline/disconnect synthesis. + by: String, + reason: String, + }, +} + +// --- cp/list_agents --- + +/// Params of `cp/list_agents`. v1 takes no arguments (the caller's +/// authenticated namespace is the scope); the struct exists so the params +/// object can grow filters without a wire break. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct ListAgentsParams {} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct AgentSummary { + pub name: String, + #[serde(rename = "type")] + pub agent_type: AgentType, + pub instance_id: String, + pub labels: std::collections::BTreeMap, + pub active_sessions: u32, + pub max_delegated_sessions: u32, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ListAgentsResult { + pub namespace: String, + pub agents: Vec, +} + // --- method names --- pub mod methods { @@ -452,6 +564,10 @@ pub mod methods { pub const DELEGATE: &str = "cp/delegate"; pub const DELEGATE_RESULT: &str = "cp/delegate_result"; pub const CANCEL: &str = "cp/cancel"; + /// CP → observer notification carrying an [`EventParams`] payload. + pub const EVENT: &str = "cp/event"; + /// Namespace-scoped registry snapshot (any registered client). + pub const LIST_AGENTS: &str = "cp/list_agents"; } #[cfg(test)] @@ -742,6 +858,57 @@ mod tests { assert!(resp.method.is_none() && resp.result.is_some()); } + #[test] + fn observer_type_roundtrip() { + let json = serde_json::json!({ + "protocol_version": 1, + "namespace": "prod", + "name": "lobby-app", + "type": "observer", + "instance_id": "i-app" + }); + let p: RegisterParams = serde_json::from_value(json).unwrap(); + assert_eq!(p.agent_type, AgentType::Observer); + assert_eq!(serde_json::to_value(&p.agent_type).unwrap(), "observer"); + } + + #[test] + fn event_notification_has_no_id_and_flattens_event() { + let ev = EventParams { + seq: 7, + ts: chrono::Utc::now(), + namespace: "prod".into(), + event: CpEvent::DelegationRequested { + delegation_id: "d-1".into(), + from: "prod/koudu".into(), + to: "prod/worker-1".into(), + prompt_excerpt: "do it".into(), + deadline: chrono::Utc::now(), + chain: vec!["prod/koudu".into()], + }, + }; + let n = JsonRpcNotification::new(methods::EVENT, serde_json::to_value(&ev).unwrap()); + let v = serde_json::to_value(&n).unwrap(); + assert_eq!(v["method"], "cp/event"); + assert!(v.get("id").is_none(), "notifications carry no id"); + assert_eq!(v["params"]["event"], "delegation_requested"); + assert_eq!(v["params"]["seq"], 7); + assert_eq!(v["params"]["from"], "prod/koudu"); + } + + #[test] + fn event_tag_snake_case() { + let ev = CpEvent::AgentDeregistered { + agent: "prod/w1".into(), + instance_id: "i-1".into(), + reason: "lease_expired".into(), + }; + let v = serde_json::to_value(&ev).unwrap(); + assert_eq!(v["event"], "agent_deregistered"); + let back: CpEvent = serde_json::from_value(v).unwrap(); + assert_eq!(back, ev); + } + #[test] fn request_envelope_validation() { let ok: JsonRpcMessage = diff --git a/crates/openab-cp/src/registry.rs b/crates/openab-cp/src/registry.rs index 7404bf0d4..c24e67f4e 100644 --- a/crates/openab-cp/src/registry.rs +++ b/crates/openab-cp/src/registry.rs @@ -327,7 +327,8 @@ impl Registry { /// Select a serving instance within `namespace` by exact name or labels. /// - /// Unsaturated matches only. Ordering: + /// Observers are never selectable: they are read-only lobby clients, + /// not delegation targets. Unsaturated matches only. Ordering: /// - exact-name selection → replicas of one logical agent: newest /// registration first (rolling-deploy rule), load as tie-breaker /// - label selection → across logical agents: least loaded first, @@ -342,6 +343,7 @@ impl Registry { let mut matches: Vec<&Instance> = g .values() .map(|e| &e.inst) + .filter(|i| i.agent_type != AgentType::Observer) .filter(|i| i.namespace == namespace) .filter(|i| match name { Some(n) => i.name == n, @@ -394,6 +396,17 @@ impl Registry { .cloned() .collect() } + + /// Observer connections in one namespace — the `cp/event` fan-out set. + pub fn observers(&self, namespace: &str) -> Vec { + self.inner + .read() + .values() + .map(|e| &e.inst) + .filter(|i| i.agent_type == AgentType::Observer && i.namespace == namespace) + .cloned() + .collect() + } } #[derive(Debug, PartialEq, Eq)] @@ -590,6 +603,24 @@ mod tests { assert_eq!(r.expired(Duration::ZERO), vec![h]); } + #[test] + fn observers_never_selectable_but_listed() { + let r = Registry::new(); + let mut ob = inst("prod", "lobby", "i-app", 0); + ob.agent_type = AgentType::Observer; + r.register(ob); + // Even an exact-name selection cannot route to an observer. + assert!(matches!( + r.select("prod", Some("lobby"), None), + Err(SelectError::NoTarget) + )); + // Observer fan-out set is namespace-scoped. + assert_eq!(r.observers("prod").len(), 1); + assert!(r.observers("dev").is_empty()); + // list() still shows it (lobby sees itself in the roster). + assert_eq!(r.list("prod").len(), 1); + } + #[test] fn colliding_instance_id_cannot_replace_other_registration() { // A second connection registering the same client-supplied From e2c35b0c5291289e48a27f914d4fc5c41f13f9dd Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Tue, 11 Aug 2026 20:55:20 -0400 Subject: [PATCH 2/9] feat(cp): wire observer events, per-namespace seq, cp/list_agents (Phase 1 complete) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns the Phase 1 observer scaffold into a working lobby surface: events are now actually emitted, sequenced, and readable, and observers can fetch a namespace roster. Wire-type changes (nothing has shipped; the wire is still free): - EventParams.seq is now documented and implemented as PER-NAMESPACE monotonic. A process-global counter manufactures false gaps for an observer whenever another namespace is busy, which would break the "gap means I lost frames, resync via cp/list_agents" contract. - AgentDeregistered.reason: String -> DeregisterReason enum {disconnect, lease_expired} (snake_case, Display impl). The CP is the only producer, so an open string bought nothing. - DelegationCancelled gains from/to: an observer that missed delegation_requested (late join, dropped frame) still gets full attribution. - DelegationRequested.prompt_excerpt is now Option with skip_serializing_if, symmetric with result_excerpt, so metadata_only namespaces omit the key entirely instead of shipping an empty string. New config knob: - [namespaces.X] metadata_only (bool, default false — previous behavior). When true, cp/event carries no payload bodies: no prompt_excerpt, no result_excerpt, and no runtime-reported error body. Attribution (from/to/chain), timing, seq, and status are unaffected. Documented in cp.toml.example. Event hub (new src/events.rs, owned by AppState): - Per-namespace seq counters; one serialization per emission, fanned out to registry.observers(ns) via the existing bounded per-connection queue with try_send. The delegation path is never awaited or failed for a lobby client: a saturated observer queue drops the frame (and the observer sees the seq gap). - No observers in the namespace -> nothing serialized and no seq consumed, which keeps an observer's stream dense from its very first frame. - Excerpts reuse the router's marker-inside-the-cap truncation helper (extracted as router::truncate_with_marker, previously inlined in complete()), so result capping and event excerpts share one UTF-8-boundary-safe implementation. Post-review fix (audit finding): seq allocation and observer enqueue are now atomic per namespace — the per-namespace stream lock is held from seq allocation through the last try_send, so concurrent emits cannot enqueue out of order and false-trigger gap detection. Sends inside the lock are non-blocking try_sends to bounded queues, so hold time is bounded and the delegation path never waits on an observer. Regression test concurrent_emits_enqueue_in_seq_order (8 threads x 16 events) fails 5/5 under the racy pattern, passes under this one. --- crates/openab-cp/cp.toml.example | 7 +- crates/openab-cp/src/config.rs | 32 ++ crates/openab-cp/src/events.rs | 407 +++++++++++++++++++ crates/openab-cp/src/lib.rs | 1 + crates/openab-cp/src/policy.rs | 4 + crates/openab-cp/src/proto.rs | 104 ++++- crates/openab-cp/src/router.rs | 665 +++++++++++++++++++++++++++---- crates/openab-cp/src/server.rs | 425 ++++++++++++++++++-- 8 files changed, 1532 insertions(+), 113 deletions(-) create mode 100644 crates/openab-cp/src/events.rs diff --git a/crates/openab-cp/cp.toml.example b/crates/openab-cp/cp.toml.example index 7b15c959c..a0d735473 100644 --- a/crates/openab-cp/cp.toml.example +++ b/crates/openab-cp/cp.toml.example @@ -101,7 +101,12 @@ name = "lobby-app" type = "observer" # Per-namespace policy. Absent namespaces use the conservative defaults: -# max_depth = 1, allow_worker_initiation = false. +# max_depth = 1, allow_worker_initiation = false, metadata_only = false. [namespaces.prod] max_depth = 1 allow_worker_initiation = false +# Withhold prompt/result bodies from cp/event: observers still see every +# event with full attribution (from/to/chain), timing, and status, but no +# payload excerpts. Default false (excerpts included, bounded by +# max_event_excerpt_bytes). +# metadata_only = true diff --git a/crates/openab-cp/src/config.rs b/crates/openab-cp/src/config.rs index ce81d3eb8..672e9c39f 100644 --- a/crates/openab-cp/src/config.rs +++ b/crates/openab-cp/src/config.rs @@ -218,6 +218,12 @@ pub struct NamespacePolicy { /// Whether workers may initiate delegations (depth still applies). #[serde(default)] pub allow_worker_initiation: bool, + /// Withhold prompt/result bodies from `cp/event`: observers still see + /// every event, with attribution, timing, and status, but no payload + /// excerpts. Defaults to false (excerpts included, bounded by + /// `max_event_excerpt_bytes`) — the pre-existing behavior. + #[serde(default)] + pub metadata_only: bool, } fn default_depth() -> u32 { @@ -229,6 +235,7 @@ impl Default for NamespacePolicy { Self { max_depth: default_depth(), allow_worker_initiation: false, + metadata_only: false, } } } @@ -428,6 +435,31 @@ allow_worker_initiation = false assert!(!d.allow_worker_initiation); } + #[test] + fn metadata_only_defaults_off_and_is_per_namespace() { + let cfg: CpConfig = toml::from_str(base_toml()).unwrap(); + assert!( + !cfg.policy_for("prod").metadata_only, + "backward-compatible default: excerpts are included" + ); + assert!(!cfg.policy_for("unlisted").metadata_only); + + let cfg: CpConfig = toml::from_str( + r#" +[namespaces.prod] +metadata_only = true + +[namespaces.dev] +max_depth = 3 +"#, + ) + .unwrap(); + assert!(cfg.policy_for("prod").metadata_only); + // Unrelated knobs keep their defaults when only metadata_only is set. + assert_eq!(cfg.policy_for("prod").max_depth, 1); + assert!(!cfg.policy_for("dev").metadata_only); + } + #[test] fn identity_lookup_binds_key_to_claims() { let cfg: CpConfig = toml::from_str(base_toml()).unwrap(); diff --git a/crates/openab-cp/src/events.rs b/crates/openab-cp/src/events.rs new file mode 100644 index 000000000..b71120238 --- /dev/null +++ b/crates/openab-cp/src/events.rs @@ -0,0 +1,407 @@ +//! Observer event fan-out — the "lobby" surface. +//! +//! Design invariants: +//! +//! - **Per-namespace sequence numbers.** Each namespace has its own monotonic +//! counter, so an observer sees a dense stream and a gap unambiguously +//! means "frames were dropped / the CP restarted — resync via +//! `cp/list_agents`". A process-global counter would manufacture gaps out +//! of unrelated activity in other namespaces. +//! - **Best effort, never blocking.** Fan-out uses `try_send` on the same +//! bounded per-connection queue everything else uses: an observer that +//! cannot keep up loses frames (and detects it via `seq`). The delegation +//! path is never awaited, slowed, or failed because of a lobby client. +//! - **Namespace isolation.** An event is only ever offered to observers +//! registered in that same namespace. +//! - **Bounded bodies.** Prompt/result excerpts are truncated with the same +//! marker-inside-the-cap helper the router uses for oversized results, and +//! are omitted entirely for `metadata_only` namespaces. + +use std::collections::{BTreeMap, BTreeSet}; +use std::sync::Arc; + +use parking_lot::Mutex; + +use crate::config::CpConfig; +use crate::proto::{methods, CpEvent, EventParams, JsonRpcNotification}; +use crate::registry::Registry; +use crate::router::truncate_with_marker; + +/// Serializes one `cp/event` notification per emission and offers it to every +/// observer in the target namespace. +#[derive(Debug)] +pub struct EventHub { + /// namespace → its ordered event stream. The outer lock only guards + /// map get-or-create; ordering is enforced by the inner per-namespace + /// lock (see [`EventHub::emit`]). + streams: Mutex>>>, + max_excerpt_bytes: usize, + /// Namespaces configured `metadata_only = true`. + metadata_only: BTreeSet, +} + +impl EventHub { + pub fn new(cfg: &CpConfig) -> Self { + Self { + streams: Mutex::new(BTreeMap::new()), + max_excerpt_bytes: cfg.max_event_excerpt_bytes, + metadata_only: cfg + .namespaces + .iter() + .filter(|(_, p)| p.metadata_only) + .map(|(ns, _)| ns.clone()) + .collect(), + } + } + + /// Whether payload bodies are withheld from this namespace's events. + pub fn is_metadata_only(&self, namespace: &str) -> bool { + self.metadata_only.contains(namespace) + } + + /// Bounded excerpt of an **agent-supplied** body (prompt, result, or a + /// runtime-reported error). `None` when the namespace is `metadata_only`. + pub fn excerpt(&self, namespace: &str, body: &str) -> Option { + if self.is_metadata_only(namespace) { + return None; + } + Some(truncate_with_marker(body, self.max_excerpt_bytes)) + } + + /// [`EventHub::excerpt`] over an optional body. + pub fn excerpt_opt(&self, namespace: &str, body: Option<&str>) -> Option { + body.and_then(|b| self.excerpt(namespace, b)) + } + + /// Bound a short CP-synthesized string (a cancellation reason, a timeout + /// or disconnect diagnostic). These are metadata, not payload, so + /// `metadata_only` does not suppress them. + pub fn bounded(&self, text: &str) -> String { + truncate_with_marker(text, self.max_excerpt_bytes) + } + + /// Serialize `event` once and offer it to every observer in `namespace`. + /// + /// No observers → nothing is serialized and no sequence number is + /// consumed, which keeps the stream dense from an observer's first frame. + /// + /// Ordering: the per-namespace stream lock is held from seq allocation + /// through the last `try_send`, so frames enter every observer queue in + /// seq order — concurrent emits in one namespace cannot interleave + /// (seq=2 enqueued before seq=1 would false-trigger gap detection). + /// The sends inside the lock are non-blocking `try_send`s to bounded + /// queues, so the hold time is bounded and the delegation path never + /// waits on a slow observer. The registry lock is not held here: + /// `observers()` returns a cloned snapshot. + pub fn emit(&self, registry: &Registry, namespace: &str, event: CpEvent) { + let observers = registry.observers(namespace); + if observers.is_empty() { + return; + } + let stream = { + let mut g = self.streams.lock(); + Arc::clone(g.entry(namespace.to_string()).or_default()) + }; + let mut seq = stream.lock(); + *seq += 1; + let params = EventParams { + seq: *seq, + ts: chrono::Utc::now(), + namespace: namespace.to_string(), + event, + }; + // Serialized once per emission; registry/hub map locks are not held. + let text = serde_json::to_string(&JsonRpcNotification::new( + methods::EVENT, + serde_json::to_value(¶ms).expect("serializable"), + )) + .expect("serializable"); + for o in observers { + // Best effort by design: a saturated lobby queue drops the frame. + if o.tx.try_send(text.clone()).is_err() { + tracing::debug!( + observer = %o.logical_id(), + instance = %o.instance_id, + seq = params.seq, + "observer queue full or closed — event frame dropped" + ); + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::proto::{AgentType, DeregisterReason}; + use crate::registry::{Instance, OUTBOUND_QUEUE}; + use std::time::Instant; + + fn hub(toml_str: &str) -> EventHub { + EventHub::new(&toml::from_str::(toml_str).unwrap()) + } + + fn observer(registry: &Registry, ns: &str, name: &str) -> crate::registry::FrameRx { + // Generous byte budget: these tests exercise entry-count semantics. + let (tx, rx) = crate::registry::outbound_channel(64 * 1024 * 1024); + registry.register(Instance { + handle: 0, + namespace: ns.into(), + name: name.into(), + agent_type: AgentType::Observer, + instance_id: format!("i-{name}"), + labels: Default::default(), + max_delegated_sessions: 0, + active_sessions: 0, + registered_at: Instant::now(), + last_heartbeat: Instant::now(), + tx, + }); + rx + } + + fn registered(agent: &str) -> CpEvent { + CpEvent::AgentRegistered { + agent: agent.into(), + agent_type: AgentType::Worker, + instance_id: "i-x".into(), + labels: Default::default(), + } + } + + fn drain(rx: &mut crate::registry::FrameRx) -> Vec { + let mut out = Vec::new(); + while let Ok(text) = rx.try_recv() { + out.push(serde_json::from_str(&text).unwrap()); + } + out + } + + #[test] + fn seq_is_dense_per_namespace_and_namespaces_are_isolated() { + let registry = Registry::new(); + let h = hub(""); + let mut prod = observer(®istry, "prod", "lobby-prod"); + let mut dev = observer(®istry, "dev", "lobby-dev"); + + // Interleave emissions across the two namespaces. + h.emit(®istry, "prod", registered("prod/a")); + h.emit(®istry, "dev", registered("dev/a")); + h.emit(®istry, "prod", registered("prod/b")); + h.emit(®istry, "dev", registered("dev/b")); + h.emit(®istry, "prod", registered("prod/c")); + + let prod_frames = drain(&mut prod); + let dev_frames = drain(&mut dev); + assert_eq!(prod_frames.len(), 3); + assert_eq!(dev_frames.len(), 2); + + // Each observer sees a dense 1..n stream — a global counter would + // show 1,3,5 here and 2,4 there. + for (i, f) in prod_frames.iter().enumerate() { + assert_eq!(f["params"]["seq"], (i + 1) as u64, "prod seq must be dense"); + assert_eq!(f["params"]["namespace"], "prod"); + assert!(f["params"]["agent"].as_str().unwrap().starts_with("prod/")); + } + for (i, f) in dev_frames.iter().enumerate() { + assert_eq!(f["params"]["seq"], (i + 1) as u64, "dev seq must be dense"); + assert_eq!(f["params"]["namespace"], "dev"); + assert!(f["params"]["agent"].as_str().unwrap().starts_with("dev/")); + } + } + + #[test] + fn concurrent_emits_enqueue_in_seq_order() { + // Regression (review): seq allocation and enqueue must be atomic per + // namespace. If the stream lock were released between allocating seq + // and try_send, two concurrent emits could enqueue 2 before 1 and an + // observer would false-detect a gap. 8 threads × 16 events = 128 + // frames, within OUTBOUND_QUEUE (256) so nothing drops. + let registry = Registry::new(); + let h = hub(""); + let mut rx = observer(®istry, "prod", "lobby"); + + std::thread::scope(|s| { + for t in 0..8 { + let h = &h; + let registry = ®istry; + s.spawn(move || { + for i in 0..16 { + h.emit(registry, "prod", registered(&format!("prod/t{t}-{i}"))); + } + }); + } + }); + + let frames = drain(&mut rx); + assert_eq!(frames.len(), 128, "no frame may drop below queue capacity"); + for (i, f) in frames.iter().enumerate() { + assert_eq!( + f["params"]["seq"], + (i + 1) as u64, + "received order must be strictly 1..N — enqueue happened out of seq order" + ); + } + } + + #[test] + fn observer_receives_nothing_from_another_namespace() { + let registry = Registry::new(); + let h = hub(""); + let mut a = observer(®istry, "ns-a", "lobby-a"); + h.emit(®istry, "ns-b", registered("ns-b/w1")); + assert!( + drain(&mut a).is_empty(), + "cross-namespace leakage into the lobby" + ); + // ...and ns-b's (absent) observers consumed no ns-a sequence number. + h.emit(®istry, "ns-a", registered("ns-a/w1")); + assert_eq!(drain(&mut a)[0]["params"]["seq"], 1); + } + + #[test] + fn multiple_observers_in_one_namespace_all_receive_the_same_frame() { + let registry = Registry::new(); + let h = hub(""); + let mut one = observer(®istry, "prod", "lobby-1"); + let mut two = observer(®istry, "prod", "lobby-2"); + h.emit(®istry, "prod", registered("prod/w1")); + let a = drain(&mut one); + let b = drain(&mut two); + assert_eq!(a.len(), 1); + assert_eq!(a[0]["params"]["seq"], b[0]["params"]["seq"]); + assert_eq!(a[0]["params"]["agent"], b[0]["params"]["agent"]); + } + + #[test] + fn full_observer_queue_drops_the_frame_without_error() { + let registry = Registry::new(); + let h = hub(""); + let mut slow = observer(®istry, "prod", "lobby-slow"); + let mut fast = observer(®istry, "prod", "lobby-fast"); + // Saturate the slow observer's bounded queue. + let slow_inst = registry + .observers("prod") + .into_iter() + .find(|i| i.name == "lobby-slow") + .unwrap(); + for _ in 0..OUTBOUND_QUEUE { + slow_inst.tx.try_send("filler".to_string()).unwrap(); + } + assert!(slow_inst.tx.try_send("overflow".to_string()).is_err()); + + // Emission must not panic and must still reach the healthy observer. + h.emit(®istry, "prod", registered("prod/w1")); + let fast_frames = drain(&mut fast); + assert_eq!(fast_frames.len(), 1); + assert_eq!(fast_frames[0]["params"]["seq"], 1); + // The slow observer holds only filler; the event frame was dropped. + let slow_frames: Vec = { + let mut v = Vec::new(); + while let Ok(t) = slow.try_recv() { + v.push(t); + } + v + }; + assert_eq!(slow_frames.len(), OUTBOUND_QUEUE); + assert!(slow_frames.iter().all(|t| t == "filler")); + + // A closed receiver is equally harmless. + drop(fast); + h.emit(®istry, "prod", registered("prod/w2")); + } + + #[test] + fn excerpt_truncation_is_utf8_boundary_safe_and_within_cap() { + // 2-byte and 3-byte chars, so many caps land mid-character. + let body = "héllo 世界 ".repeat(40); + for cap in 1..=90usize { + let h = hub(&format!("max_event_excerpt_bytes = {cap}")); + let out = h.excerpt("prod", &body).unwrap(); + assert!( + out.len() <= cap, + "cap {cap} exceeded: {} bytes ({out:?})", + out.len() + ); + // Returning a String at all proves no mid-char slice panicked. + assert!(out.is_char_boundary(out.len())); + } + + // A representative cap keeps the head and the marker. + let h = hub("max_event_excerpt_bytes = 96"); + let out = h.excerpt("prod", &body).unwrap(); + assert!(out.contains("truncated by control plane")); + let head = out.split('\n').next().unwrap(); + assert!(body.starts_with(head), "head must be a prefix of the body"); + + // Bodies within the cap are passed through untouched. + assert_eq!(h.excerpt("prod", "短").unwrap(), "短"); + } + + #[test] + fn metadata_only_omits_excerpts_but_keeps_metadata() { + let h = hub(r#" +[namespaces.secret] +metadata_only = true + +[namespaces.prod] +max_depth = 2 +"#); + assert!(h.is_metadata_only("secret")); + assert!(!h.is_metadata_only("prod")); + assert!(!h.is_metadata_only("unlisted")); + + assert_eq!(h.excerpt("secret", "top secret prompt"), None); + assert_eq!(h.excerpt_opt("secret", Some("top secret result")), None); + assert_eq!( + h.excerpt("prod", "visible prompt"), + Some("visible prompt".to_string()) + ); + assert_eq!(h.excerpt_opt("prod", None), None); + // CP-synthesized diagnostics are metadata and survive the knob. + assert_eq!(h.bounded("deadline exceeded"), "deadline exceeded"); + + // On the wire, the excerpt key disappears entirely. + let registry = Registry::new(); + let mut rx = observer(®istry, "secret", "lobby"); + h.emit( + ®istry, + "secret", + CpEvent::DelegationRequested { + delegation_id: "d-1".into(), + from: "secret/koudu".into(), + to: "secret/worker-1".into(), + prompt_excerpt: h.excerpt("secret", "top secret prompt"), + deadline: chrono::Utc::now(), + chain: vec!["secret/koudu".into()], + }, + ); + let f = drain(&mut rx); + assert_eq!(f[0]["params"]["event"], "delegation_requested"); + assert_eq!(f[0]["params"]["from"], "secret/koudu"); + assert!(f[0]["params"].get("prompt_excerpt").is_none()); + } + + #[test] + fn emitted_frame_is_a_notification_with_expected_envelope() { + let registry = Registry::new(); + let h = hub(""); + let mut rx = observer(®istry, "prod", "lobby"); + h.emit( + ®istry, + "prod", + CpEvent::AgentDeregistered { + agent: "prod/w1".into(), + instance_id: "i-1".into(), + reason: DeregisterReason::LeaseExpired, + }, + ); + let f = drain(&mut rx); + assert_eq!(f[0]["jsonrpc"], "2.0"); + assert_eq!(f[0]["method"], "cp/event"); + assert!(f[0].get("id").is_none(), "events are notifications"); + assert_eq!(f[0]["params"]["event"], "agent_deregistered"); + assert_eq!(f[0]["params"]["reason"], "lease_expired"); + assert!(f[0]["params"]["ts"].is_string()); + } +} diff --git a/crates/openab-cp/src/lib.rs b/crates/openab-cp/src/lib.rs index f4a6ac14f..2ffefece0 100644 --- a/crates/openab-cp/src/lib.rs +++ b/crates/openab-cp/src/lib.rs @@ -7,6 +7,7 @@ //! `cp/delegate` / `cp/delegate_result` frames between them. pub mod config; +pub mod events; pub mod policy; pub mod proto; pub mod registry; diff --git a/crates/openab-cp/src/policy.rs b/crates/openab-cp/src/policy.rs index cca9dcc48..d79c995d4 100644 --- a/crates/openab-cp/src/policy.rs +++ b/crates/openab-cp/src/policy.rs @@ -178,6 +178,7 @@ mod tests { let relaxed = NamespacePolicy { max_depth: 2, allow_worker_initiation: true, + metadata_only: false, }; assert!(check(&input, &relaxed).is_ok()); } @@ -196,6 +197,7 @@ mod tests { let relaxed = NamespacePolicy { max_depth: 99, allow_worker_initiation: true, + metadata_only: false, }; assert_eq!( check(&input, &relaxed), @@ -218,6 +220,7 @@ mod tests { let relaxed = NamespacePolicy { max_depth: 2, allow_worker_initiation: true, + metadata_only: false, }; assert!(check(&input, &relaxed).is_ok()); } @@ -229,6 +232,7 @@ mod tests { let relaxed = NamespacePolicy { max_depth: 5, allow_worker_initiation: true, + metadata_only: false, }; let input = base(now, &chain); assert!(matches!( diff --git a/crates/openab-cp/src/proto.rs b/crates/openab-cp/src/proto.rs index c813ac590..16ad36362 100644 --- a/crates/openab-cp/src/proto.rs +++ b/crates/openab-cp/src/proto.rs @@ -470,12 +470,14 @@ impl JsonRpcNotification { } /// Envelope of one `cp/event` notification. -/// -/// `seq` is a CP-process-monotonic sequence number: observers detect gaps -/// (dropped frames on a saturated queue, CP restart) by discontinuity and -/// resynchronize via `cp/list_agents`. It is not durable across CP restarts. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct EventParams { + /// **Per-namespace** monotonic sequence number: an observer sees a dense + /// `1, 2, 3, …` stream for its own namespace, so a discontinuity means + /// frames were dropped (saturated queue) or the CP restarted, and the + /// observer resynchronizes via `cp/list_agents`. A process-global counter + /// would show false gaps caused purely by activity in other namespaces. + /// Not durable across CP restarts. pub seq: u64, pub ts: chrono::DateTime, /// Namespace this event is scoped to (matches the observer's own). @@ -484,9 +486,30 @@ pub struct EventParams { pub event: CpEvent, } +/// Why an instance left the registry. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum DeregisterReason { + /// The WebSocket closed (graceful close, transport error, or a peer that + /// could not drain its outbound queue). + Disconnect, + /// Heartbeats stopped arriving and the lease window elapsed. + LeaseExpired, +} + +impl std::fmt::Display for DeregisterReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + DeregisterReason::Disconnect => write!(f, "disconnect"), + DeregisterReason::LeaseExpired => write!(f, "lease_expired"), + } + } +} + /// Lobby-visible control-plane events. Prompt/result bodies are carried as /// bounded excerpts (`max_event_excerpt_bytes`): the lobby is an audit -/// surface, not a second delivery path for full payloads. +/// surface, not a second delivery path for full payloads. Namespaces marked +/// `metadata_only` omit those excerpts entirely. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(tag = "event", rename_all = "snake_case")] pub enum CpEvent { @@ -501,14 +524,15 @@ pub enum CpEvent { AgentDeregistered { agent: String, instance_id: String, - /// `"disconnect"` or `"lease_expired"`. - reason: String, + reason: DeregisterReason, }, DelegationRequested { delegation_id: String, from: String, to: String, - prompt_excerpt: String, + /// Absent when the namespace is `metadata_only`. + #[serde(default, skip_serializing_if = "Option::is_none")] + prompt_excerpt: Option, deadline: chrono::DateTime, chain: Vec, }, @@ -524,6 +548,12 @@ pub enum CpEvent { }, DelegationCancelled { delegation_id: String, + /// Initiator of the cancelled delegation (`namespace/name`) — an + /// observer that missed `delegation_requested` still gets full + /// attribution. + from: String, + /// Serving instance of the cancelled delegation (`namespace/name`). + to: String, /// Who cancelled: the initiator's logical id, or `"control-plane"` /// for deadline/disconnect synthesis. by: String, @@ -882,7 +912,7 @@ mod tests { delegation_id: "d-1".into(), from: "prod/koudu".into(), to: "prod/worker-1".into(), - prompt_excerpt: "do it".into(), + prompt_excerpt: Some("do it".into()), deadline: chrono::Utc::now(), chain: vec!["prod/koudu".into()], }, @@ -901,7 +931,7 @@ mod tests { let ev = CpEvent::AgentDeregistered { agent: "prod/w1".into(), instance_id: "i-1".into(), - reason: "lease_expired".into(), + reason: DeregisterReason::LeaseExpired, }; let v = serde_json::to_value(&ev).unwrap(); assert_eq!(v["event"], "agent_deregistered"); @@ -909,6 +939,60 @@ mod tests { assert_eq!(back, ev); } + #[test] + fn deregister_reason_serde_roundtrip() { + for (reason, wire) in [ + (DeregisterReason::Disconnect, "disconnect"), + (DeregisterReason::LeaseExpired, "lease_expired"), + ] { + let v = serde_json::to_value(reason).unwrap(); + assert_eq!(v, serde_json::json!(wire)); + assert_eq!( + serde_json::from_value::(v).unwrap(), + reason + ); + assert_eq!(reason.to_string(), wire); + } + } + + #[test] + fn delegation_cancelled_carries_from_and_to() { + let ev = CpEvent::DelegationCancelled { + delegation_id: "d-1".into(), + from: "prod/koudu".into(), + to: "prod/worker-1".into(), + by: "control-plane".into(), + reason: "deadline exceeded".into(), + }; + let v = serde_json::to_value(&ev).unwrap(); + assert_eq!(v["event"], "delegation_cancelled"); + assert_eq!(v["from"], "prod/koudu"); + assert_eq!(v["to"], "prod/worker-1"); + assert_eq!(v["by"], "control-plane"); + let back: CpEvent = serde_json::from_value(v).unwrap(); + assert_eq!(back, ev); + } + + #[test] + fn absent_prompt_excerpt_is_omitted_from_the_wire() { + let ev = CpEvent::DelegationRequested { + delegation_id: "d-1".into(), + from: "prod/koudu".into(), + to: "prod/worker-1".into(), + prompt_excerpt: None, + deadline: chrono::Utc::now(), + chain: vec!["prod/koudu".into()], + }; + let v = serde_json::to_value(&ev).unwrap(); + assert!( + v.get("prompt_excerpt").is_none(), + "metadata-only events carry no excerpt key" + ); + // ... and the omission deserializes back to None. + let back: CpEvent = serde_json::from_value(v).unwrap(); + assert_eq!(back, ev); + } + #[test] fn request_envelope_validation() { let ok: JsonRpcMessage = diff --git a/crates/openab-cp/src/router.rs b/crates/openab-cp/src/router.rs index de86ece26..613c5639c 100644 --- a/crates/openab-cp/src/router.rs +++ b/crates/openab-cp/src/router.rs @@ -68,9 +68,10 @@ use parking_lot::Mutex; use tracing::{info, warn}; use crate::config::CpConfig; +use crate::events::EventHub; use crate::policy::{self, PolicyInput}; use crate::proto::{ - codes, methods, AgentType, CancelParams, DelegateAck, DelegateForward, DelegateParams, + codes, methods, AgentType, CancelParams, CpEvent, DelegateAck, DelegateForward, DelegateParams, DelegateResultParams, DelegationStatus, ErrorObject, JsonRpcRequest, }; use crate::registry::{Instance, Registry, SelectError}; @@ -83,15 +84,12 @@ use crate::registry::{Instance, Registry, SelectError}; pub struct InFlight { /// Namespace that owns this delegation — part of its identity, not just a /// lookup key: `delegation_id` is client-supplied and only unique within - /// the namespace that produced it. - /// - /// Stored on the entry because the delegation outlives the request that - /// created it, and the paths that end it without a client request — - /// `fail_instance` and `sweep_deadlines` — have no namespace of their own - /// to work from. Its consumer is the observer event layer added by the - /// next PR in this stack (per-namespace `cp/event` fan-out reads - /// `e.namespace` in exactly those two paths), so the field is part of the - /// entry's contract rather than an unused remnant. + /// the namespace that produced it. Both endpoints share it (v1 delegation + /// is same-namespace only), so it is also the scope of the `cp/event` + /// fan-out for this delegation: the paths that end a delegation without a + /// client request — `fail_instance` and `sweep_deadlines` — read + /// `e.namespace` to emit their terminal events. Always the authenticated + /// initiator's namespace — the same value as the in-flight key's. pub namespace: String, pub delegation_id: String, /// Authenticated initiator (`namespace/name`) and its registration handle. @@ -330,11 +328,14 @@ impl Router { } /// Handle `cp/delegate` from an authenticated, registered initiator. + /// Observers in the initiator's namespace are notified after the forward + /// leaves the CP (best effort — see [`EventHub`]). #[allow(clippy::too_many_arguments)] pub fn delegate( &self, cfg: &CpConfig, registry: &Registry, + events: &EventHub, from_namespace: &str, from_name: &str, from_type: &AgentType, @@ -617,6 +618,20 @@ impl Router { "delegation routed" ); + // Lobby fan-out: after the forward, never in its way. + events.emit( + registry, + from_namespace, + CpEvent::DelegationRequested { + delegation_id: entry.delegation_id.clone(), + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + prompt_excerpt: events.excerpt(from_namespace, &forward.prompt), + deadline: entry.deadline, + chain: entry.chain.clone(), + }, + ); + DelegateOutcome::Accepted(DelegateAck { delegation_id: params.delegation_id, admission: generation, @@ -804,9 +819,19 @@ impl Router { /// Only the instance the delegation was routed to may complete it; a /// non-owner frame can never make the delegation momentarily invisible /// to a genuine result or to the deadline sweep. + /// + /// Observers in the delegation's namespace are notified of the terminal + /// status as soon as the peek validates the frame — before the initiator + /// is even looked up — so the lobby sees the delegation end even when the + /// initiator is already gone or its queue refuses the result. Like every + /// `cp/event` fan-out this is best effort and shares the initiator's + /// "first terminal frame wins" semantics: the peek-send window can put a + /// second terminal event on the wire for one `delegation_id`, and + /// observers MUST treat the first as authoritative (see [`EventHub`]). pub fn complete( &self, registry: &Registry, + events: &EventHub, serving_handle: u64, mut params: DelegateResultParams, max_result_bytes: usize, @@ -878,18 +903,31 @@ impl Router { // exceeds max_result_bytes. if let Some(r) = ¶ms.result { if r.len() > max_result_bytes { - let marker = format!("\n…[truncated by control plane: {} bytes total]", r.len()); - let budget = max_result_bytes.saturating_sub(marker.len()); - let cut = floor_char_boundary(r, budget); - let mut out = format!("{}{}", &r[..cut], marker); - if out.len() > max_result_bytes { - // Degenerate tiny cap: keep whatever fits. - out.truncate(floor_char_boundary(&out, max_result_bytes)); - } - params.result = Some(out); + params.result = Some(truncate_with_marker(r, max_result_bytes)); } } + // Lobby fan-out: the peek has validated the entry snapshot and the + // serving handle, so the delegation HAS reached a terminal status as + // far as the CP is concerned. Emitting here — before the initiator + // lookup and its `try_send` — is deliberate: observers see the + // terminal status even when the initiator is gone or its queue + // refuses the frame. Not moved into the commit phase, which reports + // who won a race rather than what happened; observers get the same + // at-least-once, first-terminal-wins contract as initiators. + events.emit( + registry, + &entry.namespace, + CpEvent::DelegationCompleted { + delegation_id: params.delegation_id.clone(), + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + status: params.status.clone(), + result_excerpt: events.excerpt_opt(&entry.namespace, params.result.as_deref()), + error: events.excerpt_opt(&entry.namespace, params.error.as_deref()), + }, + ); + let Some(initiator) = registry.get(entry.from_handle) else { // The initiator deregistered concurrently: its `fail_instance` // pass removes this entry, releases capacity, and cancels the @@ -977,6 +1015,7 @@ impl Router { pub fn cancel( &self, registry: &Registry, + events: &EventHub, from_handle: u64, params: &CancelParams, next_rpc_id: u64, @@ -1043,6 +1082,17 @@ impl Router { admission = entry.generation, "delegation cancelled by initiator" ); + events.emit( + registry, + &entry.namespace, + CpEvent::DelegationCancelled { + delegation_id: params.delegation_id.clone(), + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + by: entry.from_logical.clone(), + reason: events.bounded(¶ms.reason), + }, + ); let target = registry.get(entry.to_handle); Ok(target.map(|t| { // Forwarded verbatim: the token was just matched against the live @@ -1065,6 +1115,7 @@ impl Router { pub fn fail_instance( &self, registry: &Registry, + events: &EventHub, handle: u64, rpc_id: &mut impl FnMut() -> u64, ) -> Vec<(Instance, String)> { @@ -1081,6 +1132,7 @@ impl Router { for e in entries { if e.to_handle == handle { // Serving side died → tell the initiator. + let error = format!("{} disconnected", e.to_logical); if let Some(init) = registry.get(e.from_handle) { let params = DelegateResultParams { delegation_id: e.delegation_id.clone(), @@ -1090,7 +1142,7 @@ impl Router { admission: e.generation, status: DelegationStatus::TargetDisconnected, result: None, - error: Some(format!("{} disconnected", e.to_logical)), + error: Some(error.clone()), }; let frame = JsonRpcRequest::new( rpc_id(), @@ -1099,9 +1151,24 @@ impl Router { ); affected.push((init, serde_json::to_string(&frame).expect("serializable"))); } + // Emitted even when the initiator is already gone: the lobby + // must see the delegation reach a terminal state. + events.emit( + registry, + &e.namespace, + CpEvent::DelegationCompleted { + delegation_id: e.delegation_id.clone(), + from: e.from_logical.clone(), + to: e.to_logical.clone(), + status: DelegationStatus::TargetDisconnected, + result_excerpt: None, + error: Some(events.bounded(&error)), + }, + ); } else { // Initiator died → cancel downstream, free worker capacity. registry.adjust_sessions(e.to_handle, -1); + let reason = format!("initiator {} disconnected", e.from_logical); if let Some(target) = registry.get(e.to_handle) { let params = CancelParams { delegation_id: e.delegation_id.clone(), @@ -1110,7 +1177,7 @@ impl Router { // this best-effort frame reaches the worker, the frame // still names the admission that is over. admission: e.generation, - reason: format!("initiator {} disconnected", e.from_logical), + reason: reason.clone(), }; let frame = JsonRpcRequest::new( rpc_id(), @@ -1119,6 +1186,17 @@ impl Router { ); affected.push((target, serde_json::to_string(&frame).expect("serializable"))); } + events.emit( + registry, + &e.namespace, + CpEvent::DelegationCancelled { + delegation_id: e.delegation_id.clone(), + from: e.from_logical.clone(), + to: e.to_logical.clone(), + by: "control-plane".to_string(), + reason: events.bounded(&reason), + }, + ); } warn!(delegation = %e.delegation_id, handle, "in-flight delegation failed by disconnect"); } @@ -1130,6 +1208,7 @@ impl Router { pub fn sweep_deadlines( &self, registry: &Registry, + events: &EventHub, now: DateTime, rpc_id: &mut impl FnMut() -> u64, ) -> Vec<(Instance, String)> { @@ -1146,6 +1225,18 @@ impl Router { for e in overdue { registry.adjust_sessions(e.to_handle, -1); warn!(delegation = %e.delegation_id, deadline = %e.deadline, "delegation deadline exceeded"); + events.emit( + registry, + &e.namespace, + CpEvent::DelegationCompleted { + delegation_id: e.delegation_id.clone(), + from: e.from_logical.clone(), + to: e.to_logical.clone(), + status: DelegationStatus::Timeout, + result_excerpt: None, + error: Some(events.bounded("deadline exceeded")), + }, + ); if let Some(init) = registry.get(e.from_handle) { let params = DelegateResultParams { delegation_id: e.delegation_id.clone(), @@ -1201,6 +1292,26 @@ impl Router { } } +/// Truncate `s` to at most `cap` bytes, keeping the head and appending a +/// marker. The marker counts against the cap: the returned value never +/// exceeds `cap` (review round-2 F5), and cuts always land on UTF-8 char +/// boundaries. Shared by result capping and observer excerpts so both use +/// one implementation. +pub(crate) fn truncate_with_marker(s: &str, cap: usize) -> String { + if s.len() <= cap { + return s.to_string(); + } + let marker = format!("\n…[truncated by control plane: {} bytes total]", s.len()); + let budget = cap.saturating_sub(marker.len()); + let cut = floor_char_boundary(s, budget); + let mut out = format!("{}{}", &s[..cut], marker); + if out.len() > cap { + // Degenerate tiny cap: keep whatever fits. + out.truncate(floor_char_boundary(&out, cap)); + } + out +} + /// Largest index `<= max` that lands on a char boundary of `s`. fn floor_char_boundary(s: &str, max: usize) -> usize { let mut cut = max.min(s.len()); @@ -1308,6 +1419,7 @@ type = "worker" struct World { cfg: CpConfig, + events: EventHub, registry: Registry, router: Router, h_primary: u64, @@ -1317,13 +1429,18 @@ type = "worker" } fn world() -> World { + world_with_cfg(cfg()) + } + + fn world_with_cfg(cfg: CpConfig) -> World { let registry = Registry::new(); let (p, primary_rx) = instance("prod", "koudu", AgentType::Primary, 4); let (w, worker_rx) = instance("prod", "worker-1", AgentType::Worker, 1); let h_primary = registry.register(p); let h_worker = registry.register(w); World { - cfg: cfg(), + events: EventHub::new(&cfg), + cfg, registry, router: Router::new(), h_primary, @@ -1333,10 +1450,29 @@ type = "worker" } } + /// Register a lobby observer in `ns` and return its outbound queue. + fn observe(w: &World, ns: &str) -> crate::registry::FrameRx { + let (ob, rx) = instance(ns, "lobby", AgentType::Observer, 0); + w.registry.register(ob); + rx + } + + /// Every `cp/event` params object queued for an observer. + fn events_of(rx: &mut crate::registry::FrameRx) -> Vec { + let mut out = Vec::new(); + while let Ok(text) = rx.try_recv() { + let v: serde_json::Value = serde_json::from_str(&text).unwrap(); + assert_eq!(v["method"], "cp/event"); + out.push(v["params"].clone()); + } + out + } + fn do_delegate(w: &World, params: DelegateParams) -> DelegateOutcome { w.router.delegate( &w.cfg, &w.registry, + &w.events, "prod", "koudu", &AgentType::Primary, @@ -1389,7 +1525,7 @@ type = "worker" reason: "changed my mind".into(), }; w.router - .cancel(&w.registry, w.h_primary, &cancel, 2) + .cancel(&w.registry, &w.events, w.h_primary, &cancel, 2) .expect("the initiator may cancel"); drain(&mut w); @@ -1408,6 +1544,7 @@ type = "worker" assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", a.admission, "A's stale payload"), 1024, @@ -1436,6 +1573,7 @@ type = "worker" assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", b.admission, "B's genuine result"), 1024, @@ -1480,9 +1618,12 @@ type = "worker" id += 1; id }; - let swept = - w.router - .sweep_deadlines(&w.registry, Utc::now() + Duration::seconds(120), &mut next); + let swept = w.router.sweep_deadlines( + &w.registry, + &w.events, + Utc::now() + Duration::seconds(120), + &mut next, + ); assert_eq!(w.router.inflight_count(), 0, "A expired"); assert_eq!(w.registry.get(w.h_worker).unwrap().active_sessions, 0); @@ -1547,7 +1688,7 @@ type = "worker" reason: "changed my mind".into(), }; w.router - .cancel(&w.registry, w.h_primary, &first, 2) + .cancel(&w.registry, &w.events, w.h_primary, &first, 2) .expect("the initiator may cancel its live admission"); drain(&mut w); @@ -1561,7 +1702,7 @@ type = "worker" // The retry: same initiator, same id, A's token. let err = w .router - .cancel(&w.registry, w.h_primary, &first, 3) + .cancel(&w.registry, &w.events, w.h_primary, &first, 3) .expect_err("a cancel naming a superseded admission must be refused"); assert_eq!(err.code, codes::POLICY_DENIED); // Byte-identical to the unknown-id refusal: the retry learns nothing @@ -1575,7 +1716,7 @@ type = "worker" serde_json::to_string(&err).unwrap(), serde_json::to_string( &w.router - .cancel(&w.registry, w.h_primary, &unknown, 4) + .cancel(&w.registry, &w.events, w.h_primary, &unknown, 4) .unwrap_err() ) .unwrap(), @@ -1598,6 +1739,7 @@ type = "worker" assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", b.admission, "B's genuine result"), 1024, @@ -1630,9 +1772,12 @@ type = "worker" seq += 1; seq }; - let swept = - w.router - .sweep_deadlines(&w.registry, Utc::now() + Duration::seconds(3600), &mut next); + let swept = w.router.sweep_deadlines( + &w.registry, + &w.events, + Utc::now() + Duration::seconds(3600), + &mut next, + ); let timeout = swept .iter() .map(|(_, f)| f.clone()) @@ -1653,6 +1798,7 @@ type = "worker" assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", b.admission, "B done"), 1024, @@ -1672,7 +1818,9 @@ type = "worker" let c = accept(do_delegate(&w, delegate_params("d-2", "worker-1", 60))); drain(&mut w); w.registry.deregister(w.h_worker); - let frames = w.router.fail_instance(&w.registry, w.h_worker, &mut next); + let frames = w + .router + .fail_instance(&w.registry, &w.events, w.h_worker, &mut next); assert_eq!(frames.len(), 1); assert!(frames[0].1.contains("target_disconnected")); assert_eq!( @@ -1706,6 +1854,7 @@ type = "primary" cfg.validate().unwrap(); let registry = Registry::new(); let router = Router::new(); + let events = EventHub::new(&cfg); // Capacity 8 on the target, so the GLOBAL bound is the binding limit. let (p, mut primary_rx) = instance("prod", "koudu", AgentType::Primary, 8); let (wk, mut worker_rx) = instance("prod", "worker-1", AgentType::Worker, 8); @@ -1715,6 +1864,7 @@ type = "primary" router.delegate( &cfg, ®istry, + &events, "prod", "koudu", &AgentType::Primary, @@ -1748,6 +1898,7 @@ type = "primary" assert_eq!( router.complete( ®istry, + &events, hw, result_of("d-1", first.admission, "done"), 1024, @@ -1765,7 +1916,9 @@ type = "primary" admission: second.admission, reason: "no longer needed".into(), }; - router.cancel(®istry, hp, &cancel, 3).expect("owned"); + router + .cancel(®istry, &events, hp, &cancel, 3) + .expect("owned"); assert_eq!(router.inflight_count(), 0); assert_ne!(second.admission, accept(go("d-3")).admission); drain_all(); @@ -1777,14 +1930,19 @@ type = "primary" seq }; assert!(!router - .sweep_deadlines(®istry, Utc::now() + Duration::seconds(3600), &mut next) + .sweep_deadlines( + ®istry, + &events, + Utc::now() + Duration::seconds(3600), + &mut next + ) .is_empty()); assert_eq!(router.inflight_count(), 0); accept(go("d-4")); drain_all(); // 4. fail_instance (the initiator's own disconnect). - router.fail_instance(®istry, hp, &mut next); + router.fail_instance(®istry, &events, hp, &mut next); assert_eq!(router.inflight_count(), 0); accept(go("d-5")); drain_all(); @@ -1794,7 +1952,7 @@ type = "primary" // must come back with it, which a second target proves. let (wk2, mut worker2_rx) = instance("prod", "worker-2", AgentType::Worker, 8); registry.register(wk2); - router.fail_instance(®istry, hp, &mut next); + router.fail_instance(®istry, &events, hp, &mut next); assert_eq!(router.inflight_count(), 0); worker_rx.close(); match go("d-6") { @@ -1805,6 +1963,7 @@ type = "primary" match router.delegate( &cfg, ®istry, + &events, "prod", "koudu", &AgentType::Primary, @@ -1849,9 +2008,12 @@ type = "primary" error: None, }; assert_eq!( - w.router.complete(&w.registry, w.h_worker, result, 1024, 2), + w.router + .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), CompleteOutcome::Delivered { committed: true } ); + // Delivery lands on the initiator's own queue — the frame arriving on + // `primary_rx` IS the `init.handle == h_primary` assertion. let frame = w.primary_rx.try_recv().unwrap(); assert!(frame.contains("\"completed\"")); // The initiator-bound terminal frame names the admission it ends. @@ -1879,7 +2041,8 @@ type = "primary" error: None, }; assert_eq!( - w.router.complete(&w.registry, w.h_worker, result, 1024, 2), + w.router + .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), CompleteOutcome::Delivered { committed: true } ); w.worker_rx.try_recv().unwrap(); @@ -1915,6 +2078,7 @@ type = "primary" let registry = Registry::new(); let router = Router::new(); let cfg = cfg(); + let events = EventHub::new(&cfg); let (p1, _p1_rx) = instance("prod", "koudu", AgentType::Primary, 4); let (p2, _p2_rx) = instance("prod", "koudu-2", AgentType::Primary, 4); let (wk, mut worker_rx) = instance("prod", "worker-1", AgentType::Worker, 4); @@ -1926,6 +2090,7 @@ type = "primary" match router.delegate( &cfg, ®istry, + &events, "prod", "koudu", &AgentType::Primary, @@ -1947,12 +2112,13 @@ type = "primary" gate.wait(); // p2 dies while its delegate call is in flight. let mut next = || 99; - router.fail_instance(®istry, hp2, &mut next); + router.fail_instance(®istry, &events, hp2, &mut next); }); gate.wait(); let _ = router.delegate( &cfg, ®istry, + &events, "prod", "koudu-2", &AgentType::Primary, @@ -1994,6 +2160,7 @@ type = "primary" assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", tok, "late"), 1024, @@ -2013,7 +2180,9 @@ type = "primary" // The stalled initiator is then failed (disconnect path): capacity // is released exactly once and the serving side is told to cancel. let mut next = || 3; - let frames = w.router.fail_instance(&w.registry, w.h_primary, &mut next); + let frames = w + .router + .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); assert_eq!(frames.len(), 1); assert!(frames[0].1.contains("cp/cancel")); assert_eq!(w.router.inflight_count(), 0); @@ -2069,6 +2238,7 @@ type = "primary" let out = w.router.delegate( &w.cfg, &w.registry, + &w.events, "prod", "worker-1", &AgentType::Worker, @@ -2107,7 +2277,8 @@ type = "primary" }; // h_primary is a valid handle but NOT the serving instance. assert_eq!( - w.router.complete(&w.registry, w.h_primary, result, 1024, 2), + w.router + .complete(&w.registry, &w.events, w.h_primary, result, 1024, 2), CompleteOutcome::Dropped ); assert_eq!(w.router.inflight_count(), 1); @@ -2126,7 +2297,8 @@ type = "primary" error: None, }; assert_eq!( - w.router.complete(&w.registry, w.h_worker, result, 1024, 2), + w.router + .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), CompleteOutcome::Dropped ); } @@ -2148,7 +2320,8 @@ type = "primary" }; let cap = 96usize; assert_eq!( - w.router.complete(&w.registry, w.h_worker, result, cap, 2), + w.router + .complete(&w.registry, &w.events, w.h_worker, result, cap, 2), CompleteOutcome::Delivered { committed: true } ); let frame = w.primary_rx.try_recv().unwrap(); @@ -2176,7 +2349,8 @@ type = "primary" error: None, }; assert_eq!( - w.router.complete(&w.registry, w.h_worker, result2, 8, 3), + w.router + .complete(&w.registry, &w.events, w.h_worker, result2, 8, 3), CompleteOutcome::Delivered { committed: true } ); let frame2 = w.primary_rx.try_recv().unwrap(); @@ -2200,11 +2374,14 @@ type = "primary" }; assert!(w .router - .sweep_deadlines(&w.registry, Utc::now(), &mut next) + .sweep_deadlines(&w.registry, &w.events, Utc::now(), &mut next) .is_empty()); - let frames = - w.router - .sweep_deadlines(&w.registry, Utc::now() + Duration::seconds(120), &mut next); + let frames = w.router.sweep_deadlines( + &w.registry, + &w.events, + Utc::now() + Duration::seconds(120), + &mut next, + ); assert_eq!(frames.len(), 2); assert_eq!(w.router.inflight_count(), 0); assert_eq!(w.registry.get(w.h_worker).unwrap().active_sessions, 0); @@ -2237,7 +2414,9 @@ type = "primary" id += 1; id }; - let frames = w.router.fail_instance(&w.registry, w.h_worker, &mut next); + let frames = w + .router + .fail_instance(&w.registry, &w.events, w.h_worker, &mut next); assert_eq!(frames.len(), 1); let (inst, frame) = &frames[0]; assert_eq!(inst.handle, w.h_primary); @@ -2262,7 +2441,9 @@ type = "primary" id += 1; id }; - let frames = w.router.fail_instance(&w.registry, w.h_primary, &mut next); + let frames = w + .router + .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); assert_eq!(frames.len(), 1); let (inst, frame) = &frames[0]; assert_eq!(inst.handle, w.h_worker); @@ -2286,13 +2467,13 @@ type = "primary" }; let err = w .router - .cancel(&w.registry, w.h_worker, ¶ms, 5) + .cancel(&w.registry, &w.events, w.h_worker, ¶ms, 5) .unwrap_err(); assert_eq!(err.code, codes::POLICY_DENIED); assert_eq!(w.router.inflight_count(), 1); let fwd = w .router - .cancel(&w.registry, w.h_primary, ¶ms, 6) + .cancel(&w.registry, &w.events, w.h_primary, ¶ms, 6) .unwrap(); let (inst, frame) = fwd.unwrap(); assert_eq!(inst.handle, w.h_worker); @@ -2327,6 +2508,7 @@ allow_worker_initiation = true let root = accept(w.router.delegate( &cfg, &w.registry, + &w.events, "prod", "koudu", &AgentType::Primary, @@ -2348,6 +2530,7 @@ allow_worker_initiation = true match w.router.delegate( &cfg, &w.registry, + &w.events, "prod", "worker-2", &AgentType::Worker, @@ -2367,6 +2550,7 @@ allow_worker_initiation = true let child_ack = accept(w.router.delegate( &cfg, &w.registry, + &w.events, "prod", "worker-1", &AgentType::Worker, @@ -2386,6 +2570,7 @@ allow_worker_initiation = true match w.router.delegate( &cfg, &w.registry, + &w.events, "prod", "worker-2", &AgentType::Worker, @@ -2462,14 +2647,24 @@ allow_worker_initiation = true params: DelegateParams, rpc: u64, ) -> DelegateOutcome { - w.router - .delegate(cfg, &w.registry, "prod", name, ty, handle, params, rpc) + w.router.delegate( + cfg, + &w.registry, + &w.events, + "prod", + name, + ty, + handle, + params, + rpc, + ) } fn cancel_admission(w: &World, delegation_id: &str, admission: u64, rpc: u64) { w.router .cancel( &w.registry, + &w.events, w.h_primary, &CancelParams { delegation_id: delegation_id.into(), @@ -2610,9 +2805,12 @@ allow_worker_initiation = true id += 1; id }; - let swept = - w.router - .sweep_deadlines(&w.registry, Utc::now() + Duration::seconds(60), &mut next); + let swept = w.router.sweep_deadlines( + &w.registry, + &w.events, + Utc::now() + Duration::seconds(60), + &mut next, + ); assert!(!swept.is_empty(), "A expired and was swept"); assert_eq!(w.router.inflight_count(), 0); assert_eq!(w.registry.get(w.h_worker).unwrap().active_sessions, 0); @@ -2861,6 +3059,7 @@ allow_worker_initiation = true assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_primary, result_of("d-1", tok, "spoofed"), 1024, @@ -2878,6 +3077,7 @@ allow_worker_initiation = true assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", tok, "genuine"), 1024, @@ -2896,6 +3096,7 @@ allow_worker_initiation = true assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_primary, result_of("d-1", tok, "spoofed"), 1024, @@ -2929,6 +3130,7 @@ allow_worker_initiation = true // Registered, but not the serving instance. w.router.complete( &w.registry, + &w.events, w.h_primary, result_of("d-1", tok, "spoofed"), 1024, @@ -2938,6 +3140,7 @@ allow_worker_initiation = true gate.wait(); let genuine = w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", tok, "genuine"), 1024, @@ -2974,12 +3177,14 @@ allow_worker_initiation = true let spoof = s.spawn(|| { gate.wait(); // Registered, but not the initiator. - w.router.cancel(&w.registry, w.h_worker, ¶ms, 1).is_ok() + w.router + .cancel(&w.registry, &w.events, w.h_worker, ¶ms, 1) + .is_ok() }); gate.wait(); let genuine = w .router - .cancel(&w.registry, w.h_primary, ¶ms, 2) + .cancel(&w.registry, &w.events, w.h_primary, ¶ms, 2) .is_ok(); (spoof.join().unwrap(), genuine) }); @@ -3010,7 +3215,7 @@ allow_worker_initiation = true }; assert!(w .router - .cancel(&w.registry, w.h_worker, ¶ms, 1) + .cancel(&w.registry, &w.events, w.h_worker, ¶ms, 1) .is_err()); assert_eq!(w.router.inflight_count(), 1); assert_eq!( @@ -3021,7 +3226,7 @@ allow_worker_initiation = true // The genuine initiator can still cancel. let fwd = w .router - .cancel(&w.registry, w.h_primary, ¶ms, 2) + .cancel(&w.registry, &w.events, w.h_primary, ¶ms, 2) .unwrap() .unwrap(); assert_eq!(fwd.0.handle, w.h_worker); @@ -3052,11 +3257,11 @@ allow_worker_initiation = true // Both probes come from the worker: it initiated neither. let e_unknown = w .router - .cancel(&w.registry, w.h_worker, &unknown, 1) + .cancel(&w.registry, &w.events, w.h_worker, &unknown, 1) .unwrap_err(); let e_foreign = w .router - .cancel(&w.registry, w.h_worker, &foreign, 2) + .cancel(&w.registry, &w.events, w.h_worker, &foreign, 2) .unwrap_err(); // This one comes from the genuine initiator, naming a token that is // not the live admission's. @@ -3067,7 +3272,7 @@ allow_worker_initiation = true }; let e_stale = w .router - .cancel(&w.registry, w.h_primary, &stale, 3) + .cancel(&w.registry, &w.events, w.h_primary, &stale, 3) .unwrap_err(); let as_json = |e: &ErrorObject| serde_json::to_string(e).unwrap(); assert_eq!( @@ -3096,6 +3301,7 @@ allow_worker_initiation = true let registry = Registry::new(); let router = Router::new(); let cfg = cfg(); + let events = EventHub::new(&cfg); let (p_prod, mut prod_init_rx) = instance("prod", "koudu", AgentType::Primary, 4); let (w_prod, mut prod_rx) = instance("prod", "worker-1", AgentType::Worker, 2); let (p_dev, mut dev_init_rx) = instance("dev", "koudu", AgentType::Primary, 4); @@ -3109,6 +3315,7 @@ allow_worker_initiation = true match router.delegate( &cfg, ®istry, + &events, ns, "koudu", &AgentType::Primary, @@ -3145,11 +3352,11 @@ allow_worker_initiation = true reason: "probe".into(), }; let e_cross = router - .cancel(®istry, hw_dev, &probe, 10) + .cancel(®istry, &events, hw_dev, &probe, 10) .unwrap_err() .message; let e_nowhere = router - .cancel(®istry, hw_dev, &nowhere, 11) + .cancel(®istry, &events, hw_dev, &nowhere, 11) .unwrap_err() .message; assert_eq!(e_cross, e_nowhere); @@ -3159,6 +3366,7 @@ allow_worker_initiation = true assert_eq!( router.complete( ®istry, + &events, hw_dev, result_of("d-1", token(&router, "dev", "d-1"), "dev-done"), 1024, @@ -3166,6 +3374,8 @@ allow_worker_initiation = true ), CompleteOutcome::Delivered { committed: true } ); + // Landing on `dev_init_rx` is what "routed to hp_dev" means now that + // delivery happens inside `complete`. let frame = dev_init_rx.try_recv().unwrap(); assert!(frame.contains("dev-done")); assert!( @@ -3180,6 +3390,7 @@ allow_worker_initiation = true assert_eq!( router.complete( ®istry, + &events, hw_prod, result_of("d-1", token(&router, "prod", "d-1"), "prod-done"), 1024, @@ -3208,6 +3419,7 @@ allow_worker_initiation = true "#, ) .unwrap(); + let events = EventHub::new(&cfg); let registry = Registry::new(); let router = Router::new(); let (p_prod, _rx1) = instance("prod", "koudu", AgentType::Primary, 4); @@ -3226,6 +3438,7 @@ allow_worker_initiation = true let root = accept(router.delegate( &cfg, ®istry, + &events, "prod", "koudu", &AgentType::Primary, @@ -3242,6 +3455,7 @@ allow_worker_initiation = true match router.delegate( &cfg, ®istry, + &events, "dev", "worker-1", &AgentType::Worker, @@ -3261,6 +3475,7 @@ allow_worker_initiation = true router.delegate( &cfg, ®istry, + &events, "prod", "worker-1", &AgentType::Worker, @@ -3308,7 +3523,7 @@ allow_worker_initiation = true reason: "changed my mind".into(), }; w.router - .cancel(&w.registry, w.h_primary, ¶ms, 90) + .cancel(&w.registry, &w.events, w.h_primary, ¶ms, 90) .expect("the initiator may cancel") .map(|(_, frame)| frame) .into_iter() @@ -3322,6 +3537,7 @@ allow_worker_initiation = true }; let frames = w.router.sweep_deadlines( &w.registry, + &w.events, Utc::now() + Duration::seconds(3600), &mut next, ); @@ -3413,6 +3629,7 @@ allow_worker_initiation = true assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", b.generation, "genuine"), 1024, @@ -3554,6 +3771,7 @@ allow_worker_initiation = true assert_eq!( w.router.complete( &w.registry, + &w.events, w.h_worker, result_of("d-1", a.generation, "late"), 1024, @@ -3603,6 +3821,7 @@ allow_worker_initiation = true match w.router.delegate( &w.cfg, &w.registry, + &w.events, "dev", "koudu", &AgentType::Primary, @@ -3620,4 +3839,314 @@ allow_worker_initiation = true } dev_rx.try_recv().unwrap(); } + + // --- observer / lobby event emission (Phase 1) --- + + #[test] + fn delegate_emits_requested_event_with_bounded_prompt_excerpt() { + let mut w = world_with_cfg( + toml::from_str( + r#" +max_event_excerpt_bytes = 64 + +[[agents]] +key = "kp" +namespace = "prod" +name = "koudu" +type = "primary" +"#, + ) + .unwrap(), + ); + let mut lobby = observe(&w, "prod"); + let mut p = delegate_params("d-1", "worker-1", 60); + p.prompt = "私はとても長いプロンプトです".repeat(20); + let prompt_len = p.prompt.len(); + assert!(matches!(do_delegate(&w, p), DelegateOutcome::Accepted(_))); + + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 1); + assert_eq!(ev[0]["event"], "delegation_requested"); + assert_eq!(ev[0]["seq"], 1); + assert_eq!(ev[0]["namespace"], "prod"); + assert_eq!(ev[0]["delegation_id"], "d-1"); + assert_eq!(ev[0]["from"], "prod/koudu"); + assert_eq!(ev[0]["to"], "prod/worker-1"); + assert_eq!(ev[0]["chain"], serde_json::json!(["prod/koudu"])); + let excerpt = ev[0]["prompt_excerpt"].as_str().unwrap(); + assert!(excerpt.len() <= 64, "excerpt must respect the cap"); + assert!(excerpt.contains("truncated by control plane")); + assert!(prompt_len > 64); + + // The worker still received the FULL prompt: the lobby is a mirror, + // not a filter on the delegation path. + let fwd: serde_json::Value = + serde_json::from_str(&w.worker_rx.try_recv().unwrap()).unwrap(); + assert_eq!(fwd["params"]["prompt"].as_str().unwrap().len(), prompt_len); + } + + #[test] + fn result_emits_completed_event() { + let w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + let result = DelegateResultParams { + delegation_id: "d-1".into(), + admission: token(&w.router, "prod", "d-1"), + status: DelegationStatus::Completed, + result: Some("all done".into()), + error: None, + }; + assert_eq!( + w.router + .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), + CompleteOutcome::Delivered { committed: true } + ); + + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2, "requested + completed"); + assert_eq!(ev[1]["event"], "delegation_completed"); + assert_eq!(ev[1]["seq"], 2, "per-namespace seq stays dense"); + assert_eq!(ev[1]["status"], "completed"); + assert_eq!(ev[1]["from"], "prod/koudu"); + assert_eq!(ev[1]["to"], "prod/worker-1"); + assert_eq!(ev[1]["result_excerpt"], "all done"); + assert!(ev[1].get("error").is_none()); + } + + #[test] + fn completed_event_emitted_even_when_initiator_is_gone() { + let w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.registry.deregister(w.h_primary); + let result = DelegateResultParams { + delegation_id: "d-1".into(), + admission: token(&w.router, "prod", "d-1"), + status: DelegationStatus::Failed, + result: None, + error: Some("boom".into()), + }; + assert_eq!( + w.router + .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), + CompleteOutcome::Dropped, + "nobody left to receive the result" + ); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2); + assert_eq!(ev[1]["event"], "delegation_completed"); + assert_eq!(ev[1]["status"], "failed"); + assert_eq!(ev[1]["error"], "boom"); + } + + #[test] + fn cancel_emits_cancelled_event_with_from_and_to() { + let w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + let params = CancelParams { + delegation_id: "d-1".into(), + admission: token(&w.router, "prod", "d-1"), + reason: "changed my mind".into(), + }; + // A denied cancel emits nothing. + assert!(w + .router + .cancel(&w.registry, &w.events, w.h_worker, ¶ms, 5) + .is_err()); + assert_eq!(events_of(&mut lobby).len(), 1, "only delegation_requested"); + + assert!(w + .router + .cancel(&w.registry, &w.events, w.h_primary, ¶ms, 6) + .is_ok()); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 1); + assert_eq!(ev[0]["event"], "delegation_cancelled"); + assert_eq!(ev[0]["seq"], 2); + assert_eq!(ev[0]["from"], "prod/koudu"); + assert_eq!(ev[0]["to"], "prod/worker-1"); + assert_eq!(ev[0]["by"], "prod/koudu"); + assert_eq!(ev[0]["reason"], "changed my mind"); + } + + #[test] + fn deadline_sweep_emits_timeout_completion() { + let mut w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.worker_rx.try_recv().unwrap(); + let mut id = 100u64; + let mut next = || { + id += 1; + id + }; + w.router.sweep_deadlines( + &w.registry, + &w.events, + Utc::now() + Duration::seconds(120), + &mut next, + ); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2); + assert_eq!(ev[1]["event"], "delegation_completed"); + assert_eq!(ev[1]["status"], "timeout"); + assert_eq!(ev[1]["error"], "deadline exceeded"); + } + + #[test] + fn target_disconnect_emits_target_disconnected_completion() { + let w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.registry.deregister(w.h_worker); + let mut id = 0u64; + let mut next = || { + id += 1; + id + }; + w.router + .fail_instance(&w.registry, &w.events, w.h_worker, &mut next); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2); + assert_eq!(ev[1]["event"], "delegation_completed"); + assert_eq!(ev[1]["status"], "target_disconnected"); + assert_eq!(ev[1]["error"], "prod/worker-1 disconnected"); + } + + #[test] + fn initiator_disconnect_emits_control_plane_cancellation() { + let w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.registry.deregister(w.h_primary); + let mut id = 0u64; + let mut next = || { + id += 1; + id + }; + w.router + .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2); + assert_eq!(ev[1]["event"], "delegation_cancelled"); + assert_eq!(ev[1]["by"], "control-plane"); + assert_eq!(ev[1]["from"], "prod/koudu"); + assert_eq!(ev[1]["to"], "prod/worker-1"); + assert!(ev[1]["reason"] + .as_str() + .unwrap() + .contains("initiator prod/koudu disconnected")); + } + + #[test] + fn metadata_only_namespace_omits_prompt_and_result_excerpts() { + let w = world_with_cfg( + toml::from_str( + r#" +[[agents]] +key = "kp" +namespace = "prod" +name = "koudu" +type = "primary" + +[namespaces.prod] +metadata_only = true +"#, + ) + .unwrap(), + ); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + let result = DelegateResultParams { + delegation_id: "d-1".into(), + admission: token(&w.router, "prod", "d-1"), + status: DelegationStatus::Completed, + result: Some("secret output".into()), + error: Some("secret error".into()), + }; + assert_eq!( + w.router + .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), + CompleteOutcome::Delivered { committed: true } + ); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2); + assert!(ev[0].get("prompt_excerpt").is_none()); + assert_eq!(ev[0]["to"], "prod/worker-1", "metadata still present"); + assert!(ev[1].get("result_excerpt").is_none()); + assert!( + ev[1].get("error").is_none(), + "runtime-reported error bodies are payload too" + ); + assert_eq!(ev[1]["status"], "completed"); + } + + #[test] + fn observer_in_another_namespace_sees_nothing() { + let w = world(); + let mut other = observe(&w, "dev"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + assert!(events_of(&mut other).is_empty()); + } + + #[test] + fn saturated_observer_queue_never_affects_the_delegation() { + let mut w = world(); + let lobby_rx = observe(&w, "prod"); + let lobby = w + .registry + .observers("prod") + .into_iter() + .next() + .expect("observer registered"); + for _ in 0..crate::registry::OUTBOUND_QUEUE { + lobby.tx.try_send("filler".into()).unwrap(); + } + // Delegation must still be accepted, forwarded, and completed. + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + assert!(w.worker_rx.try_recv().unwrap().contains("cp/delegate")); + let result = DelegateResultParams { + delegation_id: "d-1".into(), + admission: token(&w.router, "prod", "d-1"), + status: DelegationStatus::Completed, + result: Some("done".into()), + error: None, + }; + assert_eq!( + w.router + .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), + CompleteOutcome::Delivered { committed: true } + ); + assert_eq!(w.router.inflight_count(), 0); + drop(lobby_rx); + } } diff --git a/crates/openab-cp/src/server.rs b/crates/openab-cp/src/server.rs index c4ef4f76c..2d6a530fe 100644 --- a/crates/openab-cp/src/server.rs +++ b/crates/openab-cp/src/server.rs @@ -40,10 +40,11 @@ use tokio::sync::watch; use tracing::{info, warn}; use crate::config::{AgentIdentity, CpConfig}; +use crate::events::EventHub; use crate::proto::{ - codes, methods, CancelParams, DelegateParams, DelegateResultParams, ErrorObject, - JsonRpcErrorResponse, JsonRpcMessage, JsonRpcResponse, RegisterAck, RegisterParams, - PROTOCOL_VERSION, + codes, methods, AgentSummary, AgentType, CancelParams, CpEvent, DelegateParams, + DelegateResultParams, DeregisterReason, ErrorObject, JsonRpcErrorResponse, JsonRpcMessage, + JsonRpcResponse, ListAgentsResult, RegisterAck, RegisterParams, PROTOCOL_VERSION, }; use crate::registry::{outbound_channel, shutdown_signal, Instance, Registry}; use crate::router::{CompleteOutcome, DelegateOutcome, Router}; @@ -52,6 +53,8 @@ pub struct AppState { pub cfg: CpConfig, pub registry: Registry, pub router: Router, + /// Observer fan-out (`cp/event`) with per-namespace sequence numbers. + pub events: EventHub, rpc_id: AtomicU64, /// Live connections per identity (`namespace/name`), counted from the /// upgrade so pre-registration sockets are bounded too. @@ -61,6 +64,7 @@ pub struct AppState { impl AppState { pub fn new(cfg: CpConfig) -> Self { Self { + events: EventHub::new(&cfg), cfg, registry: Registry::new(), router: Router::new(), @@ -394,6 +398,10 @@ async fn handle_connection( return; } + // The lobby learns about every arrival — observers included, so one + // lobby client sees the others. + announce_registration(&state, handle); + // --- Main loop: interleave inbound frames, outbound channel, shutdown --- // // Every write goes through `send_bounded`: a `select!` arm body is not @@ -517,27 +525,63 @@ impl Drop for RegistrationGuard { } } -/// Deregister this connection's own registration (by handle — cannot touch -/// another connection's entry) and fail its in-flight delegations. +/// Announce a fresh registration to the namespace's observers. +fn announce_registration(state: &Arc, handle: u64) { + if let Some(i) = state.registry.get(handle) { + state.events.emit( + &state.registry, + &i.namespace, + CpEvent::AgentRegistered { + agent: i.logical_id(), + agent_type: i.agent_type.clone(), + instance_id: i.instance_id, + labels: i.labels, + }, + ); + } +} + +/// Deregister an instance, announce it to the lobby, and fail its in-flight +/// delegations. Shared by socket teardown and lease expiry — the only +/// difference an observer sees is the [`DeregisterReason`]. /// /// Invoked from [`RegistrationGuard::drop`], so it runs on the normal return /// path and on an unwind alike. /// /// Deliberately idempotent with the sweeper: when `sweep_leases` already ran -/// `deregister` + `fail_instance` for this handle, both calls here find -/// nothing (the registry entry and the in-flight entries are gone) and are -/// no-ops. That idempotency is a contract — `fail_instance` releases -/// capacity only for entries it actually removes, so a second pass can never -/// double-release (see the capacity note in `Router::delegate`'s rollback). -fn teardown(state: &Arc, handle: u64, identity: &AgentIdentity) { - state.registry.deregister(handle); +/// this for the handle, the `deregister` finds nothing (so no second +/// announcement is emitted) and `fail_instance` finds no in-flight entries — +/// both calls are no-ops. That idempotency is a contract: `fail_instance` +/// releases capacity only for entries it actually removes, so a second pass +/// can never double-release (see the capacity note in `Router::delegate`'s +/// rollback). +fn deregister_and_announce(state: &Arc, handle: u64, reason: DeregisterReason) { + if let Some(i) = state.registry.deregister(handle) { + // Emitted after removal: a dying connection is never a fan-out target. + state.events.emit( + &state.registry, + &i.namespace, + CpEvent::AgentDeregistered { + agent: i.logical_id(), + instance_id: i.instance_id, + reason, + }, + ); + } let mut next = || state.next_rpc_id(); - for (inst, frame) in state - .router - .fail_instance(&state.registry, handle, &mut next) + for (inst, frame) in + state + .router + .fail_instance(&state.registry, &state.events, handle, &mut next) { let _ = inst.tx.try_send(frame); } +} + +/// Deregister this connection's own registration (by handle — cannot touch +/// another connection's entry) and fail its in-flight delegations. +fn teardown(state: &Arc, handle: u64, identity: &AgentIdentity) { + deregister_and_announce(state, handle, DeregisterReason::Disconnect); info!( agent = %format!("{}/{}", identity.namespace, identity.name), handle, @@ -668,6 +712,27 @@ fn handle_frame(state: &Arc, handle: u64, text: &str) -> Option { match msg @@ -712,6 +777,7 @@ fn handle_frame(state: &Arc, handle: u64, text: &str) -> Option, handle: u64, text: &str) -> Option, handle: u64, text: &str) -> Option { let p = params_or_err!(CancelParams); - match state - .router - .cancel(&state.registry, handle, &p, state.next_rpc_id()) - { + match state.router.cancel( + &state.registry, + &state.events, + handle, + &p, + state.next_rpc_id(), + ) { Ok(forward) => { if let Some((target, frame)) = forward { let _ = target.tx.try_send(frame); @@ -803,6 +873,32 @@ fn handle_frame(state: &Arc, handle: u64, text: &str) -> Option { + let agents: Vec = state + .registry + .list(&me.namespace) + .into_iter() + .map(|i| AgentSummary { + name: i.name, + agent_type: i.agent_type, + instance_id: i.instance_id, + labels: i.labels, + active_sessions: i.active_sessions, + max_delegated_sessions: i.max_delegated_sessions, + }) + .collect(); + let result = ListAgentsResult { + namespace: me.namespace.clone(), + agents, + }; + let resp = + JsonRpcResponse::new(rpc_id, serde_json::to_value(&result).expect("serializable")); + Some(serde_json::to_string(&resp).expect("serializable")) + } other => { let resp = JsonRpcErrorResponse::new( rpc_id, @@ -821,6 +917,8 @@ fn handle_frame(state: &Arc, handle: u64, text: &str) -> Option, lease: Duration) { for handle in state.registry.expired(lease) { warn!( @@ -829,14 +927,9 @@ pub fn sweep_leases(state: &Arc, lease: Duration) { ); // Signal first: `deregister` drops the registry's side of the signal. state.registry.signal_shutdown(handle, REASON_LEASE_EXPIRED); - state.registry.deregister(handle); - let mut next = || state.next_rpc_id(); - for (inst, frame) in state - .router - .fail_instance(&state.registry, handle, &mut next) - { - let _ = inst.tx.try_send(frame); - } + // Removal, the `lease_expired` announcement, and in-flight failure + // all live in one place, shared with socket teardown. + deregister_and_announce(state, handle, DeregisterReason::LeaseExpired); } } @@ -844,18 +937,20 @@ pub fn sweep_leases(state: &Arc, lease: Duration) { pub async fn run_sweeper(state: Arc) { let mut tick = tokio::time::interval(Duration::from_secs(1)); tick.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); + let lease = Duration::from_secs(state.cfg.lease_expiry_secs); loop { tick.tick().await; - sweep_leases(&state, Duration::from_secs(state.cfg.lease_expiry_secs)); + sweep_leases(&state, lease); // Deadline sweep. let mut next = || state.next_rpc_id(); - for (inst, frame) in - state - .router - .sweep_deadlines(&state.registry, chrono::Utc::now(), &mut next) - { + for (inst, frame) in state.router.sweep_deadlines( + &state.registry, + &state.events, + chrono::Utc::now(), + &mut next, + ) { let _ = inst.tx.try_send(frame); } } @@ -1409,10 +1504,272 @@ mod tests { // once, cp/cancel to the serving runtime). assert_eq!(state.router.inflight_count(), 1); let mut next = || 9; - let frames = state.router.fail_instance(&state.registry, h_i, &mut next); + let frames = state + .router + .fail_instance(&state.registry, &state.events, h_i, &mut next); assert_eq!(frames.len(), 1); assert!(frames[0].1.contains("cp/cancel")); assert_eq!(state.registry.get(h_w).unwrap().active_sessions, 0); assert_eq!(state.router.inflight_count(), 0); } + + // --- observer / lobby wiring (Phase 1) --- + + fn join( + state: &Arc, + ns: &str, + name: &str, + ty: AgentType, + ) -> (u64, crate::registry::FrameRx) { + join_at(state, ns, name, ty, Instant::now()) + } + + fn join_at( + state: &Arc, + ns: &str, + name: &str, + ty: AgentType, + last_heartbeat: Instant, + ) -> (u64, crate::registry::FrameRx) { + let (tx, rx) = crate::registry::outbound_channel(64 * 1024 * 1024); + let handle = state.registry.register(Instance { + handle: 0, + namespace: ns.into(), + name: name.into(), + agent_type: ty, + instance_id: format!("i-{name}"), + labels: Default::default(), + max_delegated_sessions: 2, + active_sessions: 0, + registered_at: Instant::now(), + last_heartbeat, + tx, + }); + (handle, rx) + } + + fn events_of(rx: &mut crate::registry::FrameRx) -> Vec { + let mut out = Vec::new(); + while let Ok(text) = rx.try_recv() { + let v: serde_json::Value = serde_json::from_str(&text).unwrap(); + assert_eq!(v["method"], "cp/event"); + out.push(v["params"].clone()); + } + out + } + + fn call(state: &Arc, handle: u64, method: &str, params: serde_json::Value) -> String { + let frame = serde_json::json!({ + "jsonrpc": "2.0", "id": 42, "method": method, "params": params + }) + .to_string(); + handle_frame(state, handle, &frame).expect("a reply") + } + + #[test] + fn registration_is_announced_to_observers_including_other_observers() { + let state = state_with(""); + let (_, mut lobby) = join(&state, "prod", "lobby", AgentType::Observer); + let (h_lobby2, mut lobby2) = join(&state, "prod", "lobby-2", AgentType::Observer); + let (h_worker, _w_rx) = join(&state, "prod", "worker-1", AgentType::Worker); + + // An observer's own arrival is visible to the lobby (itself included). + announce_registration(&state, h_lobby2); + announce_registration(&state, h_worker); + + let seen = events_of(&mut lobby); + assert_eq!(seen.len(), 2); + assert_eq!(seen[0]["event"], "agent_registered"); + assert_eq!(seen[0]["agent"], "prod/lobby-2"); + assert_eq!(seen[0]["type"], "observer"); + assert_eq!(seen[0]["seq"], 1); + assert_eq!(seen[1]["agent"], "prod/worker-1"); + assert_eq!(seen[1]["type"], "worker"); + assert_eq!(seen[1]["instance_id"], "i-worker-1"); + assert_eq!(seen[1]["seq"], 2); + assert_eq!(events_of(&mut lobby2).len(), 2); + } + + #[test] + fn disconnect_and_lease_expiry_announce_distinct_reasons() { + let state = state_with(""); + let (_, mut lobby) = join(&state, "prod", "lobby", AgentType::Observer); + let (h_a, _rx_a) = join(&state, "prod", "worker-a", AgentType::Worker); + // worker-b stopped heartbeating five minutes ago; the lobby and + // worker-a are current, so only worker-b's lease is overdue. + let (h_b, _rx_b) = join_at( + &state, + "prod", + "worker-b", + AgentType::Worker, + Instant::now() - std::time::Duration::from_secs(300), + ); + + teardown(&state, h_a, &identity()); + sweep_leases(&state, std::time::Duration::from_secs(60)); + + let seen = events_of(&mut lobby); + assert_eq!(seen.len(), 2, "one disconnect + one lease expiry: {seen:?}"); + assert_eq!(seen[0]["event"], "agent_deregistered"); + assert_eq!(seen[0]["agent"], "prod/worker-a"); + assert_eq!(seen[0]["reason"], "disconnect"); + assert_eq!(seen[0]["seq"], 1); + assert_eq!(seen[1]["agent"], "prod/worker-b"); + assert_eq!(seen[1]["reason"], "lease_expired"); + assert_eq!(seen[1]["instance_id"], "i-worker-b"); + assert_eq!(seen[1]["seq"], 2); + assert!(state.registry.get(h_a).is_none()); + assert!(state.registry.get(h_b).is_none()); + assert_eq!( + state.registry.observers("prod").len(), + 1, + "the current observer keeps its registration" + ); + } + + #[test] + fn list_agents_returns_the_callers_namespace_roster() { + let state = state_with(""); + let (h_primary, _p) = join(&state, "prod", "koudu", AgentType::Primary); + let (h_lobby, _l) = join(&state, "prod", "lobby", AgentType::Observer); + join(&state, "dev", "other", AgentType::Worker); + + for handle in [h_primary, h_lobby] { + let reply = call(&state, handle, methods::LIST_AGENTS, serde_json::json!({})); + let v: serde_json::Value = serde_json::from_str(&reply).unwrap(); + assert_eq!(v["id"], 42); + assert_eq!(v["result"]["namespace"], "prod"); + let agents = v["result"]["agents"].as_array().unwrap(); + assert_eq!(agents.len(), 2, "dev/other must not leak: {agents:?}"); + let names: Vec<&str> = agents.iter().map(|a| a["name"].as_str().unwrap()).collect(); + assert!(names.contains(&"koudu") && names.contains(&"lobby")); + let lobby = agents.iter().find(|a| a["name"] == "lobby").unwrap(); + assert_eq!(lobby["type"], "observer"); + assert_eq!(lobby["instance_id"], "i-lobby"); + assert_eq!(lobby["active_sessions"], 0); + assert_eq!(lobby["max_delegated_sessions"], 2); + } + + // v1 takes no params: an absent params object is accepted too. + let frame = + serde_json::json!({"jsonrpc": "2.0", "id": 7, "method": "cp/list_agents"}).to_string(); + let reply = handle_frame(&state, h_primary, &frame).unwrap(); + let v: serde_json::Value = serde_json::from_str(&reply).unwrap(); + assert_eq!(v["result"]["namespace"], "prod"); + } + + #[test] + fn observers_are_rejected_from_the_delegation_methods() { + let state = state_with(""); + let (h_lobby, _l) = join(&state, "prod", "lobby", AgentType::Observer); + let (h_worker, _w) = join(&state, "prod", "worker-1", AgentType::Worker); + + for (method, params) in [ + ( + methods::DELEGATE, + serde_json::json!({ + "delegation_id": "d-1", + "target": {"name": "worker-1"}, + "prompt": "do it", + "deadline": (chrono::Utc::now() + chrono::Duration::seconds(60)).to_rfc3339() + }), + ), + ( + methods::DELEGATE_RESULT, + serde_json::json!({"delegation_id": "d-1", "status": "completed"}), + ), + ( + methods::CANCEL, + serde_json::json!({"delegation_id": "d-1", "reason": "no"}), + ), + ] { + let reply = call(&state, h_lobby, method, params); + let v: serde_json::Value = serde_json::from_str(&reply).unwrap(); + assert_eq!( + v["error"]["code"], + codes::POLICY_DENIED, + "{method} must be denied for observers: {v}" + ); + assert!(v["error"]["message"] + .as_str() + .unwrap() + .contains("read-only")); + } + + // Heartbeat and list_agents remain available to observers. + let hb = call( + &state, + h_lobby, + methods::HEARTBEAT, + serde_json::json!({"instance_id": "i-lobby"}), + ); + assert!(hb.contains("\"ok\":true")); + // A non-observer is unaffected by the guard. + let reply = call( + &state, + h_worker, + methods::CANCEL, + serde_json::json!({"delegation_id": "d-nope", "admission": 1, "reason": "x"}), + ); + let v: serde_json::Value = serde_json::from_str(&reply).unwrap(); + // The router's own refusal for an unknown id is a byte-identical + // POLICY_DENIED (review round-3 F3), so the code alone cannot tell + // the two denials apart — the message can: only the guard says + // "read-only". + assert_eq!(v["error"]["code"], codes::POLICY_DENIED, "{v}"); + let msg = v["error"]["message"].as_str().unwrap(); + assert!( + !msg.contains("read-only") && msg.contains("not in flight for this instance"), + "worker reaches the router, not the observer guard: {msg}" + ); + } + + #[test] + fn delegate_through_handle_frame_emits_lobby_events() { + let state = state_with(""); + let (h_primary, _p) = join(&state, "prod", "koudu", AgentType::Primary); + let (h_worker, mut w_rx) = join(&state, "prod", "worker-1", AgentType::Worker); + let (_, mut lobby) = join(&state, "prod", "lobby", AgentType::Observer); + + let reply = call( + &state, + h_primary, + methods::DELEGATE, + serde_json::json!({ + "delegation_id": "d-1", + "target": {"name": "worker-1"}, + "prompt": "ship it", + "deadline": (chrono::Utc::now() + chrono::Duration::seconds(60)).to_rfc3339() + }), + ); + let v: serde_json::Value = serde_json::from_str(&reply).unwrap(); + assert_eq!(v["result"]["assigned_to"], "prod/worker-1", "{v}"); + let admission = v["result"]["admission"] + .as_u64() + .expect("ack carries the token"); + let forwarded = w_rx.try_recv().unwrap(); + assert!(forwarded.contains("cp/delegate")); + + let reply = call( + &state, + h_worker, + methods::DELEGATE_RESULT, + serde_json::json!({ + "delegation_id": "d-1", + "admission": admission, + "status": "completed", + "result": "ok" + }), + ); + assert!(reply.contains("\"ok\":true")); + + let seen = events_of(&mut lobby); + assert_eq!(seen.len(), 2); + assert_eq!(seen[0]["event"], "delegation_requested"); + assert_eq!(seen[0]["prompt_excerpt"], "ship it"); + assert_eq!(seen[1]["event"], "delegation_completed"); + assert_eq!(seen[1]["result_excerpt"], "ok"); + assert_eq!(seen[0]["seq"], 1); + assert_eq!(seen[1]["seq"], 2); + } } From 7473568d690ef5d544c2d11bbd81360dbe2ff6b8 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Tue, 11 Aug 2026 22:11:05 -0400 Subject: [PATCH 3/9] docs(cp): seq baseline client contract + unpaired-deregister note Self-review findings, both documentation-level: - EventParams.seq promised a dense "1, 2, 3, ..." stream, but an observer joining mid-stream sees an arbitrary starting value (earlier observers already consumed sequence numbers). A client implementing gap detection literally from that doc would false-alarm on its first frame. The contract is now explicit: first frame received sets the baseline; only a gap after it signals loss; a seq regression means CP restart -> resync. - A registration whose ack send fails tears down after registry insertion, emitting agent_deregistered with no matching agent_registered. Harmless (roster removal of an unknown agent is a no-op, and mid-stream joiners face the same case), but now stated at the emission site instead of being discovered in production logs. --- crates/openab-cp/src/proto.rs | 10 ++++++++-- crates/openab-cp/src/server.rs | 5 ++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/openab-cp/src/proto.rs b/crates/openab-cp/src/proto.rs index 16ad36362..6de9b0165 100644 --- a/crates/openab-cp/src/proto.rs +++ b/crates/openab-cp/src/proto.rs @@ -473,11 +473,17 @@ impl JsonRpcNotification { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct EventParams { /// **Per-namespace** monotonic sequence number: an observer sees a dense - /// `1, 2, 3, …` stream for its own namespace, so a discontinuity means + /// `n, n+1, n+2, …` stream for its own namespace, so a discontinuity means /// frames were dropped (saturated queue) or the CP restarted, and the /// observer resynchronizes via `cp/list_agents`. A process-global counter /// would show false gaps caused purely by activity in other namespaces. - /// Not durable across CP restarts. + /// + /// Client contract: the **first frame received sets the baseline** — an + /// observer joining mid-stream may see any starting value (events already + /// flowed to earlier observers), and only a gap *after* that first frame + /// signals loss. A `seq` lower than the last seen value means the CP + /// restarted; treat it as a new baseline and resync. Not durable across + /// CP restarts. pub seq: u64, pub ts: chrono::DateTime, /// Namespace this event is scoped to (matches the observer's own). diff --git a/crates/openab-cp/src/server.rs b/crates/openab-cp/src/server.rs index 2d6a530fe..c2ed67555 100644 --- a/crates/openab-cp/src/server.rs +++ b/crates/openab-cp/src/server.rs @@ -399,7 +399,10 @@ async fn handle_connection( } // The lobby learns about every arrival — observers included, so one - // lobby client sees the others. + // lobby client sees the others. Announced only after a successful ack: + // if the ack send fails, teardown emits an `agent_deregistered` with no + // matching `agent_registered` — roster clients must treat removal of an + // unknown agent as a no-op (they may have joined mid-stream anyway). announce_registration(&state, handle); // --- Main loop: interleave inbound frames, outbound channel, shutdown --- From 49025874dd71e45309bdb1882feb8c110f450aee Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Fri, 14 Aug 2026 19:14:40 +0000 Subject: [PATCH 4/9] fix(cp): authoritative observer terminals, admission tokens in events, metadata_only cancel reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the five critical findings from PR review round 7: - F1: DelegationRequested/Completed/Cancelled events now carry the admission token; observers correlate on (namespace, delegation_id, admission) — a reusable id alone cannot tie a terminal to its admission (cancel-then-retry). - F2: an initiator-supplied cancel reason is agent content, not a CP diagnostic: it now flows through the metadata_only-aware excerpt path (key absent under metadata_only; counterparty still receives it verbatim). bounded() renamed cp_diagnostic() so feeding client input through the ungated path reads as wrong at the call site. - F3: delegate() drops the global admission guard once admission is committed (token minted, capacity reserved, entry inserted) — observer fan-out no longer runs inside the critical section that serializes every delegation in every namespace. - F4: complete() no longer emits delegation_completed before the initiator try_send. A stalled or gone initiator resolves through fail_instance, so observers record the same single cancelled terminal the initiator does — no completed+cancelled pair. - F5: delegation_requested is emitted before the forward (a worker cannot complete a delegation it has not seen, so completed can never precede requested), and a refused forward emits a matching cancelled terminal so no requested is left dangling. Terminals are emitted only by the path that authoritatively removes the entry; dropped/stale results emit nothing. Wire changes (pre-consumer): admission field on the three delegation event variants; DelegationCancelled.reason is now Option with skip_serializing_if, symmetric with DelegationCompleted.error. Tests: 5 new/rewritten regression tests (single-terminal stalled initiator, requested-then-rollback ordering, metadata_only cancel reason suppression, admission tokens across re-admission of one id, no event for dropped results). cargo test -p openab-cp: 136 passed, 0 failed; clippy -Dwarnings clean; rustfmt clean. --- crates/openab-cp/src/events.rs | 15 +- crates/openab-cp/src/proto.rs | 30 ++- crates/openab-cp/src/router.rs | 370 ++++++++++++++++++++++++++++----- 3 files changed, 353 insertions(+), 62 deletions(-) diff --git a/crates/openab-cp/src/events.rs b/crates/openab-cp/src/events.rs index b71120238..113a0893e 100644 --- a/crates/openab-cp/src/events.rs +++ b/crates/openab-cp/src/events.rs @@ -73,10 +73,16 @@ impl EventHub { body.and_then(|b| self.excerpt(namespace, b)) } - /// Bound a short CP-synthesized string (a cancellation reason, a timeout - /// or disconnect diagnostic). These are metadata, not payload, so + /// Bound a short **CP-synthesized** diagnostic (a timeout or disconnect + /// reason the CP itself composed). These are metadata, not payload, so /// `metadata_only` does not suppress them. - pub fn bounded(&self, text: &str) -> String { + /// + /// NEVER pass agent-supplied content here: anything that originated in a + /// client frame (prompts, results, errors, an initiator's cancel reason) + /// must go through [`EventHub::excerpt`]/[`EventHub::excerpt_opt`], which + /// honor `metadata_only`. The name is deliberately specific so a call + /// site feeding client input through it reads as wrong. + pub fn cp_diagnostic(&self, text: &str) -> String { truncate_with_marker(text, self.max_excerpt_bytes) } @@ -359,7 +365,7 @@ max_depth = 2 ); assert_eq!(h.excerpt_opt("prod", None), None); // CP-synthesized diagnostics are metadata and survive the knob. - assert_eq!(h.bounded("deadline exceeded"), "deadline exceeded"); + assert_eq!(h.cp_diagnostic("deadline exceeded"), "deadline exceeded"); // On the wire, the excerpt key disappears entirely. let registry = Registry::new(); @@ -369,6 +375,7 @@ max_depth = 2 "secret", CpEvent::DelegationRequested { delegation_id: "d-1".into(), + admission: 1, from: "secret/koudu".into(), to: "secret/worker-1".into(), prompt_excerpt: h.excerpt("secret", "top secret prompt"), diff --git a/crates/openab-cp/src/proto.rs b/crates/openab-cp/src/proto.rs index 6de9b0165..b9b733431 100644 --- a/crates/openab-cp/src/proto.rs +++ b/crates/openab-cp/src/proto.rs @@ -534,6 +534,13 @@ pub enum CpEvent { }, DelegationRequested { delegation_id: String, + /// Admission token of this admission of `delegation_id`. A delegation + /// id is legally reusable (cancel-then-retry re-admits the same id), + /// so observers MUST correlate lifecycle events on the composite key + /// `(namespace, delegation_id, admission)` — the token is what ties a + /// terminal event to the exact admission it ends, mirroring the wire + /// frames (ack/result/cancel), which all carry it. + admission: u64, from: String, to: String, /// Absent when the namespace is `metadata_only`. @@ -544,6 +551,11 @@ pub enum CpEvent { }, DelegationCompleted { delegation_id: String, + /// Admission token this terminal event ends — correlate on + /// `(namespace, delegation_id, admission)`, never on the reusable id + /// alone. First terminal event for a given admission wins; later ones + /// for that admission are duplicates. + admission: u64, from: String, to: String, status: DelegationStatus, @@ -554,6 +566,11 @@ pub enum CpEvent { }, DelegationCancelled { delegation_id: String, + /// Admission token this terminal event ends — correlate on + /// `(namespace, delegation_id, admission)`, never on the reusable id + /// alone. First terminal event for a given admission wins; later ones + /// for that admission are duplicates. + admission: u64, /// Initiator of the cancelled delegation (`namespace/name`) — an /// observer that missed `delegation_requested` still gets full /// attribution. @@ -563,7 +580,13 @@ pub enum CpEvent { /// Who cancelled: the initiator's logical id, or `"control-plane"` /// for deadline/disconnect synthesis. by: String, - reason: String, + /// Bounded excerpt of the cancel reason. An initiator-supplied reason + /// is agent content: it is suppressed entirely (key absent) when the + /// namespace is `metadata_only`, exactly like prompt/result excerpts. + /// CP-synthesized reasons (disconnect/deadline diagnostics) are + /// metadata and survive the knob. + #[serde(default, skip_serializing_if = "Option::is_none")] + reason: Option, }, } @@ -916,6 +939,7 @@ mod tests { namespace: "prod".into(), event: CpEvent::DelegationRequested { delegation_id: "d-1".into(), + admission: 1, from: "prod/koudu".into(), to: "prod/worker-1".into(), prompt_excerpt: Some("do it".into()), @@ -965,10 +989,11 @@ mod tests { fn delegation_cancelled_carries_from_and_to() { let ev = CpEvent::DelegationCancelled { delegation_id: "d-1".into(), + admission: 3, from: "prod/koudu".into(), to: "prod/worker-1".into(), by: "control-plane".into(), - reason: "deadline exceeded".into(), + reason: Some("deadline exceeded".into()), }; let v = serde_json::to_value(&ev).unwrap(); assert_eq!(v["event"], "delegation_cancelled"); @@ -983,6 +1008,7 @@ mod tests { fn absent_prompt_excerpt_is_omitted_from_the_wire() { let ev = CpEvent::DelegationRequested { delegation_id: "d-1".into(), + admission: 1, from: "prod/koudu".into(), to: "prod/worker-1".into(), prompt_excerpt: None, diff --git a/crates/openab-cp/src/router.rs b/crates/openab-cp/src/router.rs index 613c5639c..e341e7abf 100644 --- a/crates/openab-cp/src/router.rs +++ b/crates/openab-cp/src/router.rs @@ -328,8 +328,11 @@ impl Router { } /// Handle `cp/delegate` from an authenticated, registered initiator. - /// Observers in the initiator's namespace are notified after the forward - /// leaves the CP (best effort — see [`EventHub`]). + /// Observers in the initiator's namespace are notified once admission is + /// committed, BEFORE the forward is attempted — outside the admission + /// critical section (best effort — see [`EventHub`]). A forward that is + /// refused emits a matching `delegation_cancelled` terminal, so the + /// stream never carries a `requested` without a terminal. #[allow(clippy::too_many_arguments)] pub fn delegate( &self, @@ -583,6 +586,34 @@ impl Router { }; self.inflight.lock().insert(key.clone(), entry.clone()); + // Admission is committed: the token is minted, capacity is reserved, + // and the entry is inserted — the duplicate check rides the in-flight + // table, so nothing below needs the admission guard. Dropping it here + // keeps observer fan-out (serialization + per-observer try_send) out + // of the one critical section that serializes every delegation in + // every namespace: a saturated lobby must never slow admission. + drop(admission); + + // Lobby fan-out BEFORE the forward: a worker cannot complete a + // delegation it has not yet seen, so emitting here anchors the + // stream causally — `delegation_completed` can never precede + // `delegation_requested` for the same admission. The rollback below + // emits a matching terminal so a refused forward never leaves a + // `requested` dangling without one. + events.emit( + registry, + from_namespace, + CpEvent::DelegationRequested { + delegation_id: entry.delegation_id.clone(), + admission: entry.generation, + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + prompt_excerpt: events.excerpt(from_namespace, &forward.prompt), + deadline: entry.deadline, + chain: entry.chain.clone(), + }, + ); + if target.tx.try_send(text).is_err() { // Disconnected or backpressured beyond its queue: roll back. // @@ -595,12 +626,24 @@ impl Router { // hides the underflow and `saturated()` then admits new work to // an instance that is actually full. // - // Matched on the generation, not just the key: the admission lock - // happens to rule out a re-admission of this id while we are - // here, but the rollback does not need that argument to be - // correct — it removes the exact entry it inserted or nothing. + // Matched on the generation, not just the key: the rollback + // removes the exact entry it inserted or nothing. Whoever wins + // the removal also owns the terminal event, so exactly one + // authoritative terminal reaches observers for this admission. if self.remove_generation(&key, entry.generation).is_some() { registry.adjust_sessions(target.handle, -1); + events.emit( + registry, + from_namespace, + CpEvent::DelegationCancelled { + delegation_id: entry.delegation_id.clone(), + admission: entry.generation, + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + by: "control-plane".to_string(), + reason: Some(events.cp_diagnostic("target disconnected during routing")), + }, + ); } return DelegateOutcome::Rejected(ErrorObject::new( codes::TARGET_DISCONNECTED, @@ -618,20 +661,6 @@ impl Router { "delegation routed" ); - // Lobby fan-out: after the forward, never in its way. - events.emit( - registry, - from_namespace, - CpEvent::DelegationRequested { - delegation_id: entry.delegation_id.clone(), - from: entry.from_logical.clone(), - to: entry.to_logical.clone(), - prompt_excerpt: events.excerpt(from_namespace, &forward.prompt), - deadline: entry.deadline, - chain: entry.chain.clone(), - }, - ); - DelegateOutcome::Accepted(DelegateAck { delegation_id: params.delegation_id, admission: generation, @@ -821,13 +850,14 @@ impl Router { /// to a genuine result or to the deadline sweep. /// /// Observers in the delegation's namespace are notified of the terminal - /// status as soon as the peek validates the frame — before the initiator - /// is even looked up — so the lobby sees the delegation end even when the - /// initiator is already gone or its queue refuses the result. Like every - /// `cp/event` fan-out this is best effort and shares the initiator's - /// "first terminal frame wins" semantics: the peek-send window can put a - /// second terminal event on the wire for one `delegation_id`, and - /// observers MUST treat the first as authoritative (see [`EventHub`]). + /// status only when THIS path authoritatively ends the delegation + /// (`Commit::Claimed`). Every other ending — cancel, sweep, initiator or + /// target disconnect, forward rollback — emits its own terminal from its + /// own removal, so the event stream carries exactly one authoritative + /// terminal per admission and it always matches CP state: a result whose + /// delivery stalls terminates through `fail_instance` and observers see + /// `delegation_cancelled`, the same outcome the initiator records. + /// Dropped and stale results emit nothing. pub fn complete( &self, registry: &Registry, @@ -907,27 +937,6 @@ impl Router { } } - // Lobby fan-out: the peek has validated the entry snapshot and the - // serving handle, so the delegation HAS reached a terminal status as - // far as the CP is concerned. Emitting here — before the initiator - // lookup and its `try_send` — is deliberate: observers see the - // terminal status even when the initiator is gone or its queue - // refuses the frame. Not moved into the commit phase, which reports - // who won a race rather than what happened; observers get the same - // at-least-once, first-terminal-wins contract as initiators. - events.emit( - registry, - &entry.namespace, - CpEvent::DelegationCompleted { - delegation_id: params.delegation_id.clone(), - from: entry.from_logical.clone(), - to: entry.to_logical.clone(), - status: params.status.clone(), - result_excerpt: events.excerpt_opt(&entry.namespace, params.result.as_deref()), - error: events.excerpt_opt(&entry.namespace, params.error.as_deref()), - }, - ); - let Some(initiator) = registry.get(entry.from_handle) else { // The initiator deregistered concurrently: its `fail_instance` // pass removes this entry, releases capacity, and cancels the @@ -971,6 +980,27 @@ impl Router { to = %entry.from_logical, "delegation completed" ); + // Lobby fan-out AFTER the authoritative removal: this path + // owns the entry's end, so it owns the terminal event. Every + // removal path (commit here, cancel, sweep, fail_instance, + // forward rollback) emits exactly one terminal from its own + // removal, so observers never see divergent or duplicate + // terminals for one admission, and a dropped or stale result + // emits nothing at all. + events.emit( + registry, + &entry.namespace, + CpEvent::DelegationCompleted { + delegation_id: params.delegation_id.clone(), + admission: entry.generation, + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + status: params.status.clone(), + result_excerpt: events + .excerpt_opt(&entry.namespace, params.result.as_deref()), + error: events.excerpt_opt(&entry.namespace, params.error.as_deref()), + }, + ); true } Commit::Vanished => { @@ -1087,10 +1117,16 @@ impl Router { &entry.namespace, CpEvent::DelegationCancelled { delegation_id: params.delegation_id.clone(), + admission: entry.generation, from: entry.from_logical.clone(), to: entry.to_logical.clone(), by: entry.from_logical.clone(), - reason: events.bounded(¶ms.reason), + // Initiator-supplied free text is agent content, not a CP + // diagnostic: it goes through the metadata_only-aware path, + // so a `metadata_only` namespace never mirrors it to + // observers (the counterparty still receives it verbatim in + // the forwarded cp/cancel below). + reason: events.excerpt(&entry.namespace, ¶ms.reason), }, ); let target = registry.get(entry.to_handle); @@ -1158,11 +1194,12 @@ impl Router { &e.namespace, CpEvent::DelegationCompleted { delegation_id: e.delegation_id.clone(), + admission: e.generation, from: e.from_logical.clone(), to: e.to_logical.clone(), status: DelegationStatus::TargetDisconnected, result_excerpt: None, - error: Some(events.bounded(&error)), + error: Some(events.cp_diagnostic(&error)), }, ); } else { @@ -1191,10 +1228,11 @@ impl Router { &e.namespace, CpEvent::DelegationCancelled { delegation_id: e.delegation_id.clone(), + admission: e.generation, from: e.from_logical.clone(), to: e.to_logical.clone(), by: "control-plane".to_string(), - reason: events.bounded(&reason), + reason: Some(events.cp_diagnostic(&reason)), }, ); } @@ -1230,11 +1268,12 @@ impl Router { &e.namespace, CpEvent::DelegationCompleted { delegation_id: e.delegation_id.clone(), + admission: e.generation, from: e.from_logical.clone(), to: e.to_logical.clone(), status: DelegationStatus::Timeout, result_excerpt: None, - error: Some(events.bounded("deadline exceeded")), + error: Some(events.cp_diagnostic("deadline exceeded")), }, ); if let Some(init) = registry.get(e.from_handle) { @@ -3919,16 +3958,23 @@ type = "primary" #[test] fn completed_event_emitted_even_when_initiator_is_gone() { + // Renamed contract (review round-7 F4/F5): an undelivered result is + // NOT an authoritative terminal. When the initiator is gone the + // result is dropped with NO observer event; the one terminal the + // lobby sees comes from the initiator's teardown (`fail_instance`) + // and matches what actually happened: the delegation was cancelled, + // its result never delivered. let w = world(); let mut lobby = observe(&w, "prod"); assert!(matches!( do_delegate(&w, delegate_params("d-1", "worker-1", 60)), DelegateOutcome::Accepted(_) )); + let admission = token(&w.router, "prod", "d-1"); w.registry.deregister(w.h_primary); let result = DelegateResultParams { delegation_id: "d-1".into(), - admission: token(&w.router, "prod", "d-1"), + admission, status: DelegationStatus::Failed, result: None, error: Some("boom".into()), @@ -3940,10 +3986,222 @@ type = "primary" "nobody left to receive the result" ); let ev = events_of(&mut lobby); + assert_eq!( + ev.len(), + 1, + "a dropped result must not emit a terminal event" + ); + assert_eq!(ev[0]["event"], "delegation_requested"); + assert_eq!(ev[0]["admission"], admission); + + // The teardown path owns the end of the delegation and its terminal. + let mut next = || 7; + w.router + .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 1, "exactly one authoritative terminal"); + assert_eq!(ev[0]["event"], "delegation_cancelled"); + assert_eq!(ev[0]["admission"], admission); + assert_eq!(ev[0]["by"], "control-plane"); + } + + #[test] + fn stalled_initiator_produces_single_cancelled_terminal_for_observers() { + // Review round-7 F4: previously `complete()` emitted + // `delegation_completed` before the initiator try_send, so a stalled + // initiator produced completed-then-cancelled — opposite outcomes + // for the two audiences. Now the observer records exactly what the + // initiator records: one cancelled terminal from `fail_instance`. + let mut w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.worker_rx.try_recv().unwrap(); + let admission = token(&w.router, "prod", "d-1"); + + let initiator_tx = w.registry.get(w.h_primary).unwrap().tx; + while initiator_tx.try_send("filler".into()).is_ok() {} + + assert_eq!( + w.router.complete( + &w.registry, + &w.events, + w.h_worker, + result_of("d-1", admission, "late"), + 1024, + 2 + ), + CompleteOutcome::InitiatorStalled { + initiator_handle: w.h_primary + } + ); + let ev = events_of(&mut lobby); + assert_eq!( + ev.len(), + 1, + "no terminal while the delegation is unresolved" + ); + assert_eq!(ev[0]["event"], "delegation_requested"); + + let mut next = || 9; + w.router + .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); + let ev = events_of(&mut lobby); + assert_eq!( + ev.len(), + 1, + "one authoritative terminal, no completed+cancelled pair" + ); + assert_eq!(ev[0]["event"], "delegation_cancelled"); + assert_eq!(ev[0]["admission"], admission); + } + + #[test] + fn refused_forward_emits_requested_then_matching_cancelled_terminal() { + // Review round-7 F5: `delegation_requested` is emitted BEFORE the + // forward (a worker cannot complete a delegation it has not seen, so + // completed can never precede requested), and a refused forward + // emits a matching terminal so no requested is left dangling. + let w = world(); + let mut lobby = observe(&w, "prod"); + let worker_tx = w.registry.get(w.h_worker).unwrap().tx; + while worker_tx.try_send("filler".into()).is_ok() {} + + match do_delegate(&w, delegate_params("d-1", "worker-1", 60)) { + DelegateOutcome::Rejected(e) => assert_eq!(e.code, codes::TARGET_DISCONNECTED), + DelegateOutcome::Accepted(_) => panic!("expected rejection, got acceptance"), + } + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2, "requested + rollback terminal"); + assert_eq!(ev[0]["event"], "delegation_requested"); + assert_eq!(ev[1]["event"], "delegation_cancelled"); + assert_eq!( + ev[0]["admission"], ev[1]["admission"], + "terminal names the admission it ends" + ); + assert_eq!(ev[1]["by"], "control-plane"); + assert_eq!(w.router.inflight_count(), 0, "rollback removed the entry"); + assert_eq!( + w.registry.get(w.h_worker).unwrap().active_sessions, + 0, + "rollback released the reserved capacity" + ); + } + + #[test] + fn metadata_only_suppresses_client_cancel_reason_in_events() { + // Review round-7 F2: an initiator's cancel reason is agent-supplied + // free text. In a metadata_only namespace it must never reach + // observers — previously it flowed through the metadata-only-immune + // diagnostic path and leaked verbatim. + let w = world_with_cfg( + toml::from_str( + r#" +[[agents]] +key = "kp" +namespace = "prod" +name = "koudu" +type = "primary" + +[namespaces.prod] +metadata_only = true +"#, + ) + .unwrap(), + ); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + let params = CancelParams { + delegation_id: "d-1".into(), + admission: token(&w.router, "prod", "d-1"), + reason: "exfiltrate: AKIA-secret-key-material".into(), + }; + assert!(w + .router + .cancel(&w.registry, &w.events, w.h_primary, ¶ms, 5) + .is_ok()); + let ev = events_of(&mut lobby); assert_eq!(ev.len(), 2); - assert_eq!(ev[1]["event"], "delegation_completed"); - assert_eq!(ev[1]["status"], "failed"); - assert_eq!(ev[1]["error"], "boom"); + assert_eq!(ev[1]["event"], "delegation_cancelled"); + assert!( + ev[1].get("reason").is_none(), + "client-supplied cancel reason must be suppressed under metadata_only" + ); + // Attribution metadata survives the knob. + assert_eq!(ev[1]["by"], "prod/koudu"); + // No frame anywhere in the lobby stream carries the reason text. + for e in &ev { + assert!( + !e.to_string().contains("AKIA-secret-key-material"), + "cancel reason leaked into observer stream: {e}" + ); + } + } + + #[test] + fn events_carry_admission_tokens_distinguishing_readmissions_of_one_id() { + // Review round-7 F1: a delegation id is legally reusable + // (cancel-then-retry). Observers correlate on + // (namespace, delegation_id, admission); the two admissions of one + // id must be distinguishable across their full lifecycle. + let mut w = world(); + let mut lobby = observe(&w, "prod"); + + // Admission A: delegate then cancel. + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + let a = token(&w.router, "prod", "d-1"); + let cancel = CancelParams { + delegation_id: "d-1".into(), + admission: a, + reason: "retrying".into(), + }; + assert!(w + .router + .cancel(&w.registry, &w.events, w.h_primary, &cancel, 5) + .is_ok()); + w.worker_rx.try_recv().unwrap(); + + // Admission B: same id, re-admitted, completed. + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + let b = token(&w.router, "prod", "d-1"); + assert_ne!(a, b, "re-admission mints a fresh token"); + assert_eq!( + w.router.complete( + &w.registry, + &w.events, + w.h_worker, + result_of("d-1", b, "done"), + 1024, + 2 + ), + CompleteOutcome::Delivered { committed: true } + ); + + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 4); + assert_eq!(ev[0]["event"], "delegation_requested"); + assert_eq!(ev[0]["admission"], a); + assert_eq!(ev[1]["event"], "delegation_cancelled"); + assert_eq!(ev[1]["admission"], a); + assert_eq!(ev[2]["event"], "delegation_requested"); + assert_eq!(ev[2]["admission"], b); + assert_eq!(ev[3]["event"], "delegation_completed"); + assert_eq!(ev[3]["admission"], b); + // Same reusable id throughout — only the token separates A from B. + for e in &ev { + assert_eq!(e["delegation_id"], "d-1"); + } } #[test] From 8438a18d690fd5911c47a3dca0099f33112eb0c7 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Fri, 14 Aug 2026 15:31:00 -0400 Subject: [PATCH 5/9] fix(cp): single admission-time inflight read, capped cancel reason, phase extraction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the round-7 fix commit found four items still open. F36 — the admission critical section acquired the in-flight lock four times (duplicate check, global bound, parent lookup, insert). The three READS are now one acquisition returning `(duplicate, live, ParentRef)`; the insert stays a second, deliberate acquisition because target selection, policy evaluation and frame serialization sit between them, and holding the table across a registry scan plus a 256 KiB serialization would trade three cheap round-trips for a long hold every other path also needs. Two is the floor, not a missed merge, and the comment says so. Atomicity of check-then-insert never came from one in-flight acquisition anyway — it comes from the admission guard spanning both. Nothing logs or formats inside the new scope: refusal paths read their values back out and report after release. The new `ParentRef` enum also makes the three parent failure shapes distinct internally while still collapsing to one wire refusal. F2 residual — the inbound `cp/cancel` reason is now capped at the server entry. The event path already redacts it under `metadata_only` and truncates it elsewhere, but capping on arrival keeps an oversized reason out of the router and the forwarded frame entirely, matching the posture `max_prompt_bytes` takes one layer earlier on the delegate path. Regression asserts the forwarded frame carries the capped value; negative-controlled (removing the cap fails it). F37 — documented the scale-upgrade path on the O(live) scans and the per- terminal `InFlight` clone: a `handle -> keys` index and a deadline-ordered structure make both O(affected), and the reason not to add them yet is that a drifted secondary index is a silent wrong-delegation bug where a slow scan is only slow. F38 — `complete()` 171 -> 148 lines with the phases named: peek, cap, deliver, commit. The delivery step became `deliver_result` returning `Deliver::{Queued, InitiatorGone, Refused}`, whose variant docs state why a refusal must NOT emit a terminal (fail_instance owns that path) — the placement whose absence caused round-7 F4. --- crates/openab-cp/src/router.rs | 315 +++++++++++++++++++++------------ crates/openab-cp/src/server.rs | 69 +++++++- 2 files changed, 269 insertions(+), 115 deletions(-) diff --git a/crates/openab-cp/src/router.rs b/crates/openab-cp/src/router.rs index e341e7abf..309e45c36 100644 --- a/crates/openab-cp/src/router.rs +++ b/crates/openab-cp/src/router.rs @@ -202,6 +202,30 @@ pub enum DelegateOutcome { /// peek-to-commit window of its own. /// /// `cp/cancel` is this helper's only caller. +/// Outcome of resolving a `cp/delegate`'s parent reference, read under the one +/// admission-time in-flight acquisition. +/// +/// A parent reference is the coupled `(parent_delegation_id, parent_admission)` +/// pair: a delegation id alone is reusable and cannot identify the admission a +/// caller was actually forwarded, so half a pair is a client bug and the three +/// failure shapes below stay distinct *internally* while collapsing to one wire +/// refusal (see `delegate`). +enum ParentRef { + /// Both halves present and they name a live admission this caller serves. + Resolved { + chain: Vec, + deadline: DateTime, + }, + /// Neither half present: a root delegation. + Root, + /// Both halves present but they name no live admission this caller serves + /// — unknown id, wrong serving handle, or a superseded admission. Kept as + /// one variant because the wire must not distinguish them. + Unresolved, + IdWithoutToken, + TokenWithoutId, +} + enum Claim { /// The caller initiated it and named its live admission; the entry has /// already been removed. @@ -360,7 +384,49 @@ impl Router { // namespace is a different delegation, so it neither collides here // nor leaks its existence. let key = DelegationKey::new(from_namespace, ¶ms.delegation_id); - if self.inflight.lock().contains_key(&key) { + + // ONE in-flight acquisition covers all three admission-time reads: + // the duplicate check, the global bound, and parent resolution. They + // are adjacent by design. The insert further down is a second, + // deliberate acquisition rather than a missed merge — target + // selection, policy evaluation and frame serialization sit between + // the two, and holding the in-flight lock across a registry scan and + // a 256 KiB serialization would trade three cheap round-trips for a + // long hold on the table every other path also needs. Atomicity of + // "check duplicate, then insert" does not come from one in-flight + // acquisition anyway: it comes from the admission guard spanning both. + // + // Nothing formats or logs inside the scope: the refusal paths below + // read their values back out and report after the guard is released. + let (duplicate, live, parent) = { + let inflight = self.inflight.lock(); + let duplicate = inflight.contains_key(&key); + let live = inflight.len(); + let parent = match (¶ms.parent_delegation_id, params.parent_admission) { + (Some(pid), Some(padmission)) => { + let parent_key = DelegationKey::new(from_namespace, pid); + // The chain and deadline are read from the very entry that + // was validated — the `p` binding, not a second lookup. A + // re-lookup after validation would reintroduce the race the + // admission token closes. + match inflight.get(&parent_key) { + Some(p) if p.to_handle == from_handle && p.generation == padmission => { + ParentRef::Resolved { + chain: p.chain.clone(), + deadline: p.deadline, + } + } + _ => ParentRef::Unresolved, + } + } + (Some(_), None) => ParentRef::IdWithoutToken, + (None, Some(_)) => ParentRef::TokenWithoutId, + (None, None) => ParentRef::Root, + }; + (duplicate, live, parent) + }; + + if duplicate { return DelegateOutcome::Rejected(ErrorObject::new( codes::DUPLICATE_DELEGATION, format!("delegation {} is already in flight", params.delegation_id), @@ -378,7 +444,6 @@ impl Router { // the send-failure rollback all remove the entry, and the count // follows by construction. A parallel counter would be one refactor // away from drifting, and a drifted global bound wedges the whole CP. - let live = self.inflight.lock().len(); if live >= cfg.max_inflight_delegations { warn!( live, @@ -396,6 +461,45 @@ impl Router { )); } + // A parent reference is the coupled (id, admission) pair — see + // `ParentRef`. Unknown, unauthorized (wrong serving handle) and stale + // (superseded admission) parents all return the SAME error — one + // refusal shape, no enumeration. A distinguishable stale refusal would + // be an oracle telling the caller whether the id it references is + // currently re-admitted, which is CP scheduling state no frame reports. + // A half-filled pair is rejected rather than silently ignored, so a + // client that drops one half sees its bug instead of getting an + // unintended root delegation. + let (parent_chain, parent_deadline) = match parent { + ParentRef::Resolved { chain, deadline } => (chain, Some(deadline)), + ParentRef::Root => (Vec::new(), None), + ParentRef::Unresolved => { + let pid = params + .parent_delegation_id + .as_deref() + .expect("Unresolved implies a parent id was supplied"); + return DelegateOutcome::Rejected(ErrorObject::new( + codes::INVALID_PARAMS, + format!("parent delegation {pid} is not in flight for this instance"), + )); + } + ParentRef::IdWithoutToken => { + return DelegateOutcome::Rejected(ErrorObject::new( + codes::INVALID_PARAMS, + "parent_delegation_id requires parent_admission: name the \ + admission token this instance was forwarded for that parent \ + (a delegation id alone is reusable and cannot identify it)", + )) + } + ParentRef::TokenWithoutId => { + return DelegateOutcome::Rejected(ErrorObject::new( + codes::INVALID_PARAMS, + "parent_admission is meaningless without parent_delegation_id: \ + omit both for a root delegation, or send both", + )) + } + }; + // Selector sanity: exactly one of name/labels. let (sel_name, sel_labels) = (params.target.name.as_deref(), params.target.labels.as_ref()); if sel_name.is_some() == sel_labels.is_some() { @@ -405,79 +509,6 @@ impl Router { )); } - // Parent linkage: chain and deadline derive from the CP's own table, - // never from the client. Three things must hold together, and the - // admission token is the one that makes the other two sufficient: - // - // 1. the parent lives in the CALLER's namespace (scoped lookup); - // 2. the caller IS the instance serving it (`to_handle`) — otherwise - // any runtime knowing a live id could borrow its trusted chain and - // deadline budget; - // 3. the caller names the SPECIFIC admission it is serving. Without - // this, "currently serving that parent" degrades to "holds the - // connection that serves whatever wears this id now": once parent - // admission A ends (cancel, completion, or sweep) and the id is - // re-admitted as B — with a single replica, to the same worker — a - // residual child request composed against A satisfies 1 and 2 - // against B and inherits B's CP-constructed chain and B's remaining - // deadline budget, so depth, cycle, and parent-budget are evaluated - // for the wrong admission. Cancels are best effort, so the CP - // cannot delegate policing of this to the worker: a buggy or - // malicious runtime holding the serving connection could trigger it - // deliberately. - // - // The id and the token are a coupled pair, enforced here because this - // is where parent presence is decided. An id without a token is - // malformed, never a wildcard; a token without an id is malformed too, - // rather than silently ignored, so a client that drops the id sees its - // bug instead of getting an unintended root delegation. - // - // Unknown, unauthorized (wrong serving handle) and stale (superseded - // admission) parents all return the SAME error — one refusal shape, no - // enumeration. A distinguishable stale refusal would be an oracle - // telling the caller whether the id it references is currently - // re-admitted, which is CP scheduling state no frame reports. - let (parent_chain, parent_deadline) = - match (¶ms.parent_delegation_id, params.parent_admission) { - (Some(pid), Some(padmission)) => { - let parent_key = DelegationKey::new(from_namespace, pid); - // ONE acquisition of the in-flight lock resolves the parent and - // validates all three conditions, and the chain and deadline - // below are read from the very entry that was validated — the - // `p` binding, not a second lookup. A re-lookup after - // validation would reintroduce the race the token closes. - match self.inflight.lock().get(&parent_key) { - Some(p) if p.to_handle == from_handle && p.generation == padmission => { - (p.chain.clone(), Some(p.deadline)) - } - _ => { - return DelegateOutcome::Rejected(ErrorObject::new( - codes::INVALID_PARAMS, - format!( - "parent delegation {pid} is not in flight for this instance" - ), - )) - } - } - } - (Some(_), None) => { - return DelegateOutcome::Rejected(ErrorObject::new( - codes::INVALID_PARAMS, - "parent_delegation_id requires parent_admission: name the \ - admission token this instance was forwarded for that parent \ - (a delegation id alone is reusable and cannot identify it)", - )) - } - (None, Some(_)) => { - return DelegateOutcome::Rejected(ErrorObject::new( - codes::INVALID_PARAMS, - "parent_admission is meaningless without parent_delegation_id: \ - omit both for a root delegation, or send both", - )) - } - (None, None) => (Vec::new(), None), - }; - // Resolve target within the initiator's namespace (v1 boundary). let target = match registry.select(from_namespace, sel_name, sel_labels) { Ok(i) => i, @@ -928,48 +959,25 @@ impl Router { } }; - // Truncate oversized results (keep the head; delegation already - // ran). The marker counts against the cap: the final value never - // exceeds max_result_bytes. - if let Some(r) = ¶ms.result { - if r.len() > max_result_bytes { - params.result = Some(truncate_with_marker(r, max_result_bytes)); + // Phase 2 — cap the payload (in place: the capped value is what the + // initiator and the lobby both see). + cap_result(&mut params, max_result_bytes); + + // Phase 3 — deliver. Irreversible: once the frame is queued the + // initiator will see this terminal, which is why the commit that makes + // it authoritative comes after, and why a refusal here must NOT emit a + // terminal event (`fail_instance` owns that path). + match deliver_result(registry, &entry, ¶ms, next_rpc_id) { + Deliver::Queued => {} + Deliver::InitiatorGone => return CompleteOutcome::Dropped, + Deliver::Refused => { + return CompleteOutcome::InitiatorStalled { + initiator_handle: entry.from_handle, + } } } - let Some(initiator) = registry.get(entry.from_handle) else { - // The initiator deregistered concurrently: its `fail_instance` - // pass removes this entry, releases capacity, and cancels the - // serving side — nothing to do here. - warn!( - delegation = %params.delegation_id, - "result for a delegation whose initiator is gone — dropped" - ); - return CompleteOutcome::Dropped; - }; - let frame = JsonRpcRequest::new( - next_rpc_id, - methods::DELEGATE_RESULT, - Some(serde_json::to_value(¶ms).expect("serializable")), - ); - let text = serde_json::to_string(&frame).expect("serializable"); - - if initiator.tx.try_send(text).is_err() { - // Bounded-queue contract: a peer that cannot drain its queue is - // treated as disconnected, never silently skipped. The entry - // stays in flight; the caller closes the initiator, whose - // teardown fails the delegation over the `fail_instance` path. - warn!( - delegation = %params.delegation_id, - initiator = %entry.from_logical, - "initiator queue full — terminal result refused, treating initiator as disconnected" - ); - return CompleteOutcome::InitiatorStalled { - initiator_handle: entry.from_handle, - }; - } - - // Phase 2 — commit. Claims ONLY the admission that was peeked; see + // Phase 4 — commit. Claims ONLY the admission that was peeked; see // `commit_completion` for why key + serving handle is not enough. let committed = match self.commit_completion(registry, &entry) { Commit::Claimed => { @@ -1155,6 +1163,22 @@ impl Router { handle: u64, rpc_id: &mut impl FnMut() -> u64, ) -> Vec<(Instance, String)> { + // Scale note (shared with `sweep_deadlines`): this is an O(live) scan of + // the whole table plus an intermediate key `Vec`, and `complete` clones + // the full `InFlight` (chain included) per terminal. That is deliberate + // at this size: `max_inflight_delegations` caps the table at 4096 by + // default, so a scan is a few thousand comparisons under a lock held for + // microseconds, and the clone keeps the emit and frame construction off + // the lock entirely. + // + // Upgrade path when the cap is raised materially: keep two secondary + // indexes beside the primary map — `handle -> {DelegationKey}` for this + // function and a deadline-ordered structure (BTreeMap or + // a binary heap) for the sweep — so both become O(affected) instead of + // O(live). Both indexes must be maintained by the same five removal + // paths that own the primary map, which is the reason not to add them + // before the size justifies it: a drifted index is a silent + // wrong-delegation bug, where a slow scan is only slow. let mut affected = Vec::new(); let entries: Vec = { let mut g = self.inflight.lock(); @@ -1336,6 +1360,69 @@ impl Router { /// exceeds `cap` (review round-2 F5), and cuts always land on UTF-8 char /// boundaries. Shared by result capping and observer excerpts so both use /// one implementation. +/// Outcome of the delivery phase of a completion. Delivery is the irreversible +/// half of `complete`: the commit that makes a terminal authoritative — and the +/// lobby event that records it — happen only after `Queued`. +enum Deliver { + /// The frame is on the initiator's outbound queue. + Queued, + /// The initiator deregistered concurrently; its own `fail_instance` pass + /// removes the entry, releases capacity and cancels the serving side, and + /// owns the terminal event. + InitiatorGone, + /// The initiator's bounded queue refused the frame. The entry stays in + /// flight; the caller closes that connection and its teardown fails the + /// delegation over `fail_instance`, which emits the single authoritative + /// terminal. Emitting `completed` here too would publish two contradictory + /// terminals for one admission. + Refused, +} + +/// Cap an oversized result in place (keep the head — the delegation already +/// ran). The marker counts against the cap, so the final value never exceeds +/// `max_result_bytes`, and the capped value is what the initiator and the lobby +/// both see. +fn cap_result(params: &mut DelegateResultParams, max_result_bytes: usize) { + if let Some(r) = ¶ms.result { + if r.len() > max_result_bytes { + params.result = Some(truncate_with_marker(r, max_result_bytes)); + } + } +} + +/// Queue the terminal result on the initiator's connection. +fn deliver_result( + registry: &Registry, + entry: &InFlight, + params: &DelegateResultParams, + next_rpc_id: u64, +) -> Deliver { + let Some(initiator) = registry.get(entry.from_handle) else { + warn!( + delegation = %params.delegation_id, + "result for a delegation whose initiator is gone — dropped" + ); + return Deliver::InitiatorGone; + }; + let frame = JsonRpcRequest::new( + next_rpc_id, + methods::DELEGATE_RESULT, + Some(serde_json::to_value(params).expect("serializable")), + ); + let text = serde_json::to_string(&frame).expect("serializable"); + if initiator.tx.try_send(text).is_err() { + // Bounded-queue contract: a peer that cannot drain its queue is + // treated as disconnected, never silently skipped. + warn!( + delegation = %params.delegation_id, + initiator = %entry.from_logical, + "initiator queue full — terminal result refused, treating initiator as disconnected" + ); + return Deliver::Refused; + } + Deliver::Queued +} + pub(crate) fn truncate_with_marker(s: &str, cap: usize) -> String { if s.len() <= cap { return s.to_string(); diff --git a/crates/openab-cp/src/server.rs b/crates/openab-cp/src/server.rs index c2ed67555..cf7a46be3 100644 --- a/crates/openab-cp/src/server.rs +++ b/crates/openab-cp/src/server.rs @@ -855,7 +855,19 @@ fn handle_frame(state: &Arc, handle: u64, text: &str) -> Option { - let p = params_or_err!(CancelParams); + let mut p = params_or_err!(CancelParams); + // Defence in depth on initiator free text. The event path already + // redacts this string in `metadata_only` namespaces and truncates + // it elsewhere, but capping at the entry keeps an oversized reason + // from being carried through the router and the forwarded frame at + // all — the same posture `max_prompt_bytes` takes on the delegate + // path, one layer earlier than the excerpt cap. + if p.reason.len() > state.cfg.max_event_excerpt_bytes { + p.reason = crate::router::truncate_with_marker( + &p.reason, + state.cfg.max_event_excerpt_bytes, + ); + } match state.router.cancel( &state.registry, &state.events, @@ -1775,4 +1787,59 @@ mod tests { assert_eq!(seen[0]["seq"], 1); assert_eq!(seen[1]["seq"], 2); } + + #[tokio::test] + async fn an_oversized_cancel_reason_is_capped_at_the_entry() { + // Initiator free text is capped before the router or the forwarded + // frame ever sees it. The event path redacts/truncates too, but this + // keeps a multi-megabyte reason from riding through the CP at all. + let state = state_with("max_event_excerpt_bytes = 256"); + let (h_primary, _p_rx) = join(&state, "prod", "koudu", AgentType::Primary); + let (_h_worker, mut w_rx) = join(&state, "prod", "worker-1", AgentType::Worker); + + let ack = call( + &state, + h_primary, + methods::DELEGATE, + serde_json::json!({ + "delegation_id": "d-cap", + "target": {"name": "worker-1"}, + "prompt": "work", + "deadline": (chrono::Utc::now() + chrono::Duration::seconds(60)).to_rfc3339() + }), + ); + let v: serde_json::Value = serde_json::from_str(&ack).unwrap(); + let admission = v["result"]["admission"] + .as_u64() + .expect("ack carries the token"); + w_rx.try_recv().expect("forwarded"); + + let huge = "A".repeat(64 * 1024); + let reply = call( + &state, + h_primary, + methods::CANCEL, + serde_json::json!({ + "delegation_id": "d-cap", + "admission": admission, + "reason": huge + }), + ); + assert!( + reply.contains("\"ok\":true"), + "the cancel itself succeeds: {reply}" + ); + + // The forwarded cp/cancel carries the capped reason, not 64 KiB. + let forwarded = w_rx.try_recv().expect("cancel forwarded to the worker"); + assert!( + forwarded.len() < 2048, + "the forwarded cancel must not carry the untruncated reason ({} bytes)", + forwarded.len() + ); + assert!( + forwarded.contains("truncated by control plane"), + "the cap leaves its marker: {forwarded}" + ); + } } From 1580a012c36e14db3378b9581368ac404b573bcb Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 22:56:54 +0000 Subject: [PATCH 6/9] fix(review): serialize wire operations with entry removal (round-8 F39, F40) Two races shared one root: entry removal (cancel, sweep, fail_instance) was not serialized with the two irreversible wire operations - the delegation forward and the result delivery - so correctness depended on winning races the code did not control. F39 - announce/forward critical section. delegate() now holds the in-flight lock across announced=true, the delegation_requested emit, and the forward try_send, re-verifying the entry's generation first. A teardown's terminal event can no longer precede the requested it terminates (both emits serialize through entry removal), and its best-effort cp/cancel can no longer be enqueued before the forward it cancels - the worker can never start work every other party recorded as cancelled. Entries removed before the section runs were never announced: teardown and sweep now gate their observer terminals and synthesized frames on InFlight::announced (capacity release unchanged), and delegate reports the loss to the initiator instead of forwarding. F40 - commit-first completion. complete() now runs peek -> cap -> commit -> emit -> deliver. Only the path that removed the entry may put an initiator-bound terminal on the wire or emit the observer terminal, so the two audiences can no longer record opposite outcomes: a result whose commit loses to a concurrent cancel/sweep/disconnect is dropped undelivered, and the CP now produces at most one initiator-bound terminal per admission (first-terminal-wins stays as client-side defence in depth). A stalled or vanished initiator no longer flips the outcome: the delegation truthfully completed; the disconnected initiator loses the result exactly as if it had died a moment earlier. This also closes the duplicate-delivery telemetry gap (a second result now drops at peek or commit with distinct logs) and the Deliver::Refused stranding window (the entry is already committed; nothing strands). Also: cap DelegateResultParams.error at max_result_bytes alongside result (carried R6-F14); fix doc-comment misattachments introduced by the round-8 refactor (Claim vs ParentRef, truncate_with_marker vs Deliver) and add the missing ParentRef variant docs; update the module lock-hierarchy and terminal-frames documentation. Tests: 4 new regressions (result racing a concurrent cancel is dropped undelivered; teardown of an unannounced entry emits and sends nothing; sweep of an unannounced entry releases capacity silently; oversized error truncated like result); 3 updated to the commit-first contract. cargo test -p openab-cp: 141 passed, 0 failed. clippy --all-targets -D warnings clean; rustfmt clean. --- crates/openab-cp/src/router.rs | 1147 +++++++++++++++++++++----------- crates/openab-cp/src/server.rs | 95 ++- 2 files changed, 813 insertions(+), 429 deletions(-) diff --git a/crates/openab-cp/src/router.rs b/crates/openab-cp/src/router.rs index 309e45c36..b2fe8c284 100644 --- a/crates/openab-cp/src/router.rs +++ b/crates/openab-cp/src/router.rs @@ -21,29 +21,31 @@ //! //! Ending a delegation is a two-sided event: a frame goes out on the wire and //! CP state is committed. The commit is exact — it claims the one admission it -//! delivered a result for (key + serving handle + [`InFlight::generation`]) or -//! nothing at all — so CP state stays consistent under any interleaving. The -//! same admission stamp is protocol-visible as +//! is ending (key + serving handle + [`InFlight::generation`]) or nothing at +//! all — so CP state stays consistent under any interleaving. The same +//! admission stamp is protocol-visible as //! [`crate::proto::AdmissionToken`]: the serving runtime echoes it in //! `cp/delegate_result` and a frame naming a stale admission is dropped before //! anything is delivered, so exactness does not stop at the CP boundary. //! -//! The wire is a different matter: a `completed` result racing the deadline -//! sweep's synthesized `timeout` can put TWO terminal frames on the wire for -//! one admission. v1 resolves that by contract instead of CP-side suppression -//! (which would need per-id terminal state the CP deliberately does not -//! keep): **the first terminal frame for an admission token wins**, and -//! initiators MUST ignore later ones for that token. Because every -//! initiator-bound terminal frame carries the token, a frame for a superseded -//! admission is distinguishable from — and cannot mask — the live one. See the -//! v1 contract amendments in `docs/adr/agent-control-plane.md`. +//! Delivery is commit-gated: only the path that removed the entry — +//! completion commit, cancel claim, deadline sweep, disconnect teardown, or +//! the forward rollback — may put an initiator-bound terminal frame on the +//! wire or emit the observer terminal event for that admission. A result +//! whose commit loses the race is discarded undelivered, so the CP itself +//! produces at most ONE initiator-bound terminal and exactly one observer +//! terminal per announced admission, and the two always agree. The client +//! contract — **the first terminal frame for an admission token wins**, later +//! ones for that token are ignored — is retained as defence in depth (e.g. +//! frames straddling a CP restart), not as the primary mechanism. See the v1 +//! contract amendments in `docs/adr/agent-control-plane.md`. //! //! # Lock hierarchy //! //! The router holds two locks and acquires them in ONE order only: //! //! ```text -//! admission → inflight (never the reverse) +//! admission → inflight → registry (read) → event stream //! ``` //! //! `admission` serializes the whole delegate admission sequence; `inflight` @@ -56,10 +58,19 @@ //! (`complete`, `cancel`, `fail_instance`, `sweep_deadlines`, `chain_of`) //! take `inflight` alone and never touch `admission`. //! -//! Registry access is a third, independent lock owned by [`Registry`]. It is -//! always acquired and released *outside* an `inflight` critical section -//! (e.g. `registry.get(...)` completes before the table is locked), so it -//! does not participate in this hierarchy. +//! The two right-hand positions exist for exactly one code path: +//! `delegate`'s announce/forward critical section holds `inflight` across one +//! [`EventHub::emit`] (which takes a registry read snapshot and then the +//! per-namespace event stream lock) and one non-blocking `try_send` to the +//! target. That hold is what makes announcement and forwarding atomic with +//! respect to entry removal — a teardown's terminal event can never precede +//! the `requested` it terminates, and its best-effort `cp/cancel` can never +//! be enqueued before the forward it cancels. The work under the hold is +//! bounded (one serialization plus non-blocking sends). Every other path +//! acquires and releases `registry` or the stream locks strictly OUTSIDE any +//! `inflight` critical section, and neither the registry nor the event hub +//! ever calls back into the router while holding its own lock, so the order +//! above is total and acyclic. use std::collections::BTreeMap; @@ -124,6 +135,20 @@ pub struct InFlight { /// another's delegation volume. Commit matching only needs never-reuse per /// `(namespace, delegation_id)` key, which per-namespace counters give. pub generation: u64, + /// Whether this admission has been announced to observers + /// (`delegation_requested` emitted) and its forward attempted. Inserted + /// `false` and flipped to `true` inside `delegate`'s announce/forward + /// critical section — under the same in-flight lock acquisition that + /// sends the forward. + /// + /// Teardown paths (`fail_instance`, `sweep_deadlines`) that remove an + /// entry still `false` must emit NO observer terminal and send NO + /// synthesized wire frames for it: the admission was never announced to + /// observers and never forwarded to the worker, so a terminal would dangle + /// without a `requested` and a synthesized result would reach an initiator + /// whose `cp/delegate` call is itself about to return an error. Capacity + /// release still applies — the reservation was made at insert. + pub announced: bool, } /// In-flight table key: `(namespace, delegation_id)`. @@ -179,29 +204,6 @@ pub enum DelegateOutcome { Rejected(ErrorObject), } -/// Result of looking up an in-flight delegation on behalf of its claimed -/// initiator and removing it if the claim holds. -/// -/// The whole check happens under ONE acquisition of the in-flight lock, which -/// is the property that matters: an earlier version removed the entry, -/// validated ownership, then reinserted it on refusal, and a genuine frame -/// landing in that window saw an empty table and was dropped as "unknown id", -/// leaving the delegation to stall until its deadline. Here the entry is -/// either removed because the caller owns it, or never touched at all. -/// -/// Single-phase by construction — the caller acts on the returned entry -/// without going back to the table — so no window exists in which the id could -/// be re-admitted under the caller's feet. That is a statement about *this -/// operation's atomicity*, and it is deliberately NOT a claim that the cancel -/// path needs no admission identity: the frame the caller sent was composed -/// against whatever it last observed, and by the time it arrives the id may -/// legitimately hold a different admission. Atomicity keeps the CP's table -/// consistent; the [`AdmissionToken`] the caller must name keeps the operation -/// aimed at the admission it meant. Both are required, and the two-phase -/// completion path needs the token for the additional reason that it has a -/// peek-to-commit window of its own. -/// -/// `cp/cancel` is this helper's only caller. /// Outcome of resolving a `cp/delegate`'s parent reference, read under the one /// admission-time in-flight acquisition. /// @@ -222,10 +224,40 @@ enum ParentRef { /// — unknown id, wrong serving handle, or a superseded admission. Kept as /// one variant because the wire must not distinguish them. Unresolved, + /// `parent_delegation_id` present without `parent_admission` — malformed: + /// an id alone is reusable and cannot identify an admission, and treating + /// it as a wildcard would silently grant whatever admission wears the id + /// now. IdWithoutToken, + /// `parent_admission` present without `parent_delegation_id` — malformed: + /// rejected rather than ignored so a client that dropped the id sees its + /// bug instead of getting an unintended root delegation. TokenWithoutId, } +/// Result of looking up an in-flight delegation on behalf of its claimed +/// initiator and removing it if the claim holds. +/// +/// The whole check happens under ONE acquisition of the in-flight lock, which +/// is the property that matters: an earlier version removed the entry, +/// validated ownership, then reinserted it on refusal, and a genuine frame +/// landing in that window saw an empty table and was dropped as "unknown id", +/// leaving the delegation to stall until its deadline. Here the entry is +/// either removed because the caller owns it, or never touched at all. +/// +/// Single-phase by construction — the caller acts on the returned entry +/// without going back to the table — so no window exists in which the id could +/// be re-admitted under the caller's feet. That is a statement about *this +/// operation's atomicity*, and it is deliberately NOT a claim that the cancel +/// path needs no admission identity: the frame the caller sent was composed +/// against whatever it last observed, and by the time it arrives the id may +/// legitimately hold a different admission. Atomicity keeps the CP's table +/// consistent; the [`AdmissionToken`] the caller must name keeps the operation +/// aimed at the admission it meant. Both are required, and the completion +/// path needs the token for the additional reason that it has a +/// peek-to-commit window of its own. +/// +/// `cp/cancel` is this helper's only caller. enum Claim { /// The caller initiated it and named its live admission; the entry has /// already been removed. @@ -312,35 +344,37 @@ enum Commit { } /// Outcome of a `cp/delegate_result` frame (see [`Router::complete`]). -/// -/// Wire delivery and state commit are distinct events: the initiator can have -/// received the result while the CP's own bookkeeping was concluded by -/// somebody else (a concurrent cancel, sweep, or disconnect). Collapsing the -/// two hid whether this frame is the one that ended the delegation. #[derive(Debug, PartialEq, Eq)] pub enum CompleteOutcome { - /// The result reached the initiator's queue. - Delivered { - /// Whether THIS frame also committed the state transition — removed - /// the in-flight entry it peeked and released the serving instance's - /// capacity. `false` means a concurrent path had already ended the - /// delegation (or its id was re-admitted), so nothing was changed - /// here; the frame was still delivered. - committed: bool, + /// THIS frame ended the delegation: the in-flight entry was removed, the + /// serving instance's capacity released, and `delegation_completed` + /// emitted — before any delivery was attempted, so the observer terminal + /// and the initiator-bound wire frame can never disagree about the + /// outcome. + Completed { + /// Whether the terminal frame reached the initiator's queue. `false` + /// means the initiator was already gone or its bounded queue refused + /// the frame — either way the initiator is disconnected (or about to + /// be) and loses the result, exactly as it would have lost a result + /// computed a moment after its death. The delegation still truthfully + /// completed: the worker did the work. + delivered: bool, + /// Set when the initiator's bounded outbound queue refused the frame: + /// per the queue contract the caller must treat that initiator as + /// disconnected and close its connection. Its teardown finds no + /// in-flight entry (this frame already committed it), so no + /// conflicting terminal is ever produced. + stalled_initiator: Option, }, /// The frame was refused or the delegation is unknown (wrong owner, - /// unknown id, unregistered caller, or the initiator is gone). Nothing - /// changed; each case is logged. + /// unknown id, stale admission, unregistered caller, or a concurrent + /// path ended the delegation first). Nothing was changed and nothing was + /// delivered; each case is logged. The concurrent-removal case is the + /// important one: whoever removed the entry owns BOTH terminals — the + /// initiator-bound wire frame and the observer event — so this result is + /// discarded rather than delivered against a terminal that already + /// happened. Dropped, - /// The initiator's bounded outbound queue refused the terminal result. - /// The entry is still in flight: the caller must treat the initiator as - /// disconnected (close its connection), whose teardown then fails the - /// delegation through `fail_instance` — capacity is released exactly - /// once and the serving runtime receives `cp/cancel`. - InitiatorStalled { - /// Registration handle of the stalled initiator. - initiator_handle: u64, - }, } impl Router { @@ -352,11 +386,14 @@ impl Router { } /// Handle `cp/delegate` from an authenticated, registered initiator. - /// Observers in the initiator's namespace are notified once admission is - /// committed, BEFORE the forward is attempted — outside the admission - /// critical section (best effort — see [`EventHub`]). A forward that is + /// Once admission is committed the announce/forward critical section — + /// outside the admission lock, under one in-flight acquisition — emits + /// `delegation_requested` and forwards to the target atomically with + /// respect to entry removal, so no terminal can precede its `requested` + /// and no teardown cancel can overtake the forward. A forward that is /// refused emits a matching `delegation_cancelled` terminal, so the - /// stream never carries a `requested` without a terminal. + /// stream never carries a `requested` without a terminal; an admission + /// removed before the section runs announces and forwards nothing. #[allow(clippy::too_many_arguments)] pub fn delegate( &self, @@ -614,6 +651,10 @@ impl Router { // distinguishable from every other in this namespace for the life // of the process. generation, + // Flipped inside the announce/forward section below; a teardown + // that removes the entry before then treats the admission as + // never-announced (no observer terminal, no synthesized frames). + announced: false, }; self.inflight.lock().insert(key.clone(), entry.clone()); @@ -625,61 +666,112 @@ impl Router { // every namespace: a saturated lobby must never slow admission. drop(admission); - // Lobby fan-out BEFORE the forward: a worker cannot complete a - // delegation it has not yet seen, so emitting here anchors the - // stream causally — `delegation_completed` can never precede - // `delegation_requested` for the same admission. The rollback below - // emits a matching terminal so a refused forward never leaves a - // `requested` dangling without one. - events.emit( - registry, - from_namespace, - CpEvent::DelegationRequested { - delegation_id: entry.delegation_id.clone(), - admission: entry.generation, - from: entry.from_logical.clone(), - to: entry.to_logical.clone(), - prompt_excerpt: events.excerpt(from_namespace, &forward.prompt), - deadline: entry.deadline, - chain: entry.chain.clone(), - }, - ); + // Announce/forward critical section. One in-flight lock acquisition + // covers three things that must be atomic with respect to entry + // removal (see the module-level lock hierarchy note): + // + // 1. `announced = true` — from here on, whoever removes this entry + // owns an observer terminal for it. + // 2. The `delegation_requested` emit — under the table lock, so a + // teardown's terminal emit (which requires removing the entry, + // which requires this lock) can never reach the stream before the + // `requested` it terminates. Emitting outside the lock allowed + // `delegation_cancelled` to win the per-namespace seq race against + // `delegation_requested` — a terminal before its `requested`, an + // invalid transition for observer state machines. + // 3. The forward `try_send` — under the same lock, so a teardown's + // best-effort `cp/cancel` (sent only after its removal, which + // needs this lock) can never be enqueued to the worker before the + // forward it cancels. Outside the lock, the worker could receive + // the cancel first (ignored: unknown admission) and then the + // forward — and run work every other party had already recorded + // as cancelled, with its capacity reservation already released. + // + // The work under the lock is bounded: one serialization, one + // non-blocking `try_send` per observer, one non-blocking `try_send` + // to the target. If a teardown or the deadline sweep removed the + // entry first, the admission is over before it was ever announced: + // emit nothing, forward nothing, report the loss to the initiator + // (whose own teardown is usually what removed it). + enum Forward { + Sent, + SendFailed(InFlight), + Gone, + } + let forwarded = { + let mut g = self.inflight.lock(); + match g.get_mut(&key) { + Some(e) if e.generation == generation => { + e.announced = true; + events.emit( + registry, + from_namespace, + CpEvent::DelegationRequested { + delegation_id: entry.delegation_id.clone(), + admission: entry.generation, + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + prompt_excerpt: events.excerpt(from_namespace, &forward.prompt), + deadline: entry.deadline, + chain: entry.chain.clone(), + }, + ); + if target.tx.try_send(text).is_err() { + // Disconnected or backpressured beyond its queue: + // roll back under the same lock that verified the + // generation — the rollback removes the exact entry + // it inserted, and no other path can interleave. + let removed = g.remove(&key).expect("present under the same lock"); + Forward::SendFailed(removed) + } else { + Forward::Sent + } + } + // A teardown (initiator or target death) or the deadline + // sweep removed the entry between insert and this section. + // Whoever removed it released the capacity reservation and, + // because the entry was not yet announced, emitted no + // observer terminal and sent no synthesized frames. + _ => Forward::Gone, + } + }; - if target.tx.try_send(text).is_err() { - // Disconnected or backpressured beyond its queue: roll back. - // - // Roll back only what this call still owns. `fail_instance` and - // `sweep_deadlines` take the in-flight lock without the admission - // lock, so they can remove this very entry between the insert - // above and this branch — and whoever removes an entry also - // releases its capacity reservation. Decrementing here after a - // concurrent removal would double-release: the saturating math - // hides the underflow and `saturated()` then admits new work to - // an instance that is actually full. - // - // Matched on the generation, not just the key: the rollback - // removes the exact entry it inserted or nothing. Whoever wins - // the removal also owns the terminal event, so exactly one - // authoritative terminal reaches observers for this admission. - if self.remove_generation(&key, entry.generation).is_some() { + match forwarded { + Forward::Sent => {} + Forward::SendFailed(removed) => { registry.adjust_sessions(target.handle, -1); + // Same thread as the `requested` emit above, so the terminal + // lands after it in the stream: no dangling `requested`. events.emit( registry, from_namespace, CpEvent::DelegationCancelled { - delegation_id: entry.delegation_id.clone(), - admission: entry.generation, - from: entry.from_logical.clone(), - to: entry.to_logical.clone(), + delegation_id: removed.delegation_id.clone(), + admission: removed.generation, + from: removed.from_logical.clone(), + to: removed.to_logical.clone(), by: "control-plane".to_string(), reason: Some(events.cp_diagnostic("target disconnected during routing")), }, ); + return DelegateOutcome::Rejected(ErrorObject::new( + codes::TARGET_DISCONNECTED, + "target disconnected or unresponsive during routing", + )); + } + Forward::Gone => { + warn!( + delegation = %entry.delegation_id, + admission = entry.generation, + "admission ended by a concurrent teardown before it was \ + announced or forwarded — nothing was sent" + ); + return DelegateOutcome::Rejected(ErrorObject::new( + codes::TARGET_DISCONNECTED, + "delegation ended during routing (initiator disconnect or \ + deadline expiry won the race); nothing was forwarded", + )); } - return DelegateOutcome::Rejected(ErrorObject::new( - codes::TARGET_DISCONNECTED, - "target disconnected or unresponsive during routing", - )); } info!( @@ -699,17 +791,6 @@ impl Router { }) } - /// Remove `key` only if it still holds `generation` — the exact admission - /// the caller is acting for — under one lock acquisition. Any other entry - /// (or none) is left untouched. - fn remove_generation(&self, key: &DelegationKey, generation: u64) -> Option { - let mut g = self.inflight.lock(); - match g.get(key) { - Some(e) if e.generation == generation => g.remove(key), - _ => None, - } - } - /// Look up `delegation_id` in the caller's namespace, assert the caller /// initiated it AND that `admission` names its live admission, and remove /// the entry if so — all under one acquisition of the in-flight lock (see @@ -832,63 +913,61 @@ impl Router { /// Handle `cp/delegate_result` from the serving runtime. /// - /// The terminal result is the one frame that must never be silently - /// dropped, so delivery happens in two phases: + /// Commit-first: the state transition that ends the delegation happens + /// BEFORE the initiator-bound frame is built or sent. /// /// 1. **Peek** — validate ownership AND the echoed admission token under - /// one in-flight lock acquisition without removing the entry - /// ([`Router::peek_for_completion`]), then build and `try_send` the - /// initiator-bound frame. - /// 2. **Commit** — only after the initiator's queue accepted the frame, - /// end the delegation ([`Router::commit_completion`]): remove the - /// entry and release the serving instance's capacity, but only if the - /// live entry is still the very admission that was peeked (key + - /// serving handle + [`InFlight::generation`]). - /// - /// The token check in phase 1 is what extends admission exactness past the - /// CP boundary. Without it a late result for a cancelled admission A, - /// arriving after the same `delegation_id` was re-admitted as B to the same - /// worker, would peek B, be delivered to the initiator as B's terminal - /// frame, and then commit B (peek and commit both saw B, so B's own - /// generation matched) — releasing capacity B still occupies and leaving - /// B's genuine result to be dropped later as unknown. + /// one in-flight lock acquisition ([`Router::peek_for_completion`]). + /// 2. **Cap** — bound the payload in place. + /// 3. **Commit** — end the delegation ([`Router::commit_completion`]): + /// remove the entry and release the serving instance's capacity, but + /// only if the live entry is still the very admission that was peeked + /// (key + serving handle + [`InFlight::generation`]). A commit that + /// finds the entry gone or superseded means a concurrent cancel, + /// sweep, or disconnect ended the delegation first — that path owns + /// BOTH terminals (the initiator-bound frame and the observer event), + /// so this result is dropped, undelivered. + /// 4. **Emit + deliver** — only the claiming frame announces the terminal + /// to observers and queues the result to the initiator. /// - /// If the initiator's bounded queue refuses the frame, the entry stays - /// in flight and [`CompleteOutcome::InitiatorStalled`] tells the caller - /// to treat the initiator as disconnected (per the bounded-queue - /// contract): its teardown runs `fail_instance`, which releases capacity - /// exactly once and sends `cp/cancel` to the serving runtime. + /// Why commit before delivery: delivery is irreversible (once queued, the + /// initiator will act on the result), so whichever of the two happens + /// first is the one that can end up contradicted. The previous order — + /// deliver, then commit — let a concurrent cancel or sweep remove the + /// entry in between: the initiator had consumed a success terminal while + /// the observer stream's only terminal for that admission said + /// `cancelled`/`timeout`, and no party could detect the split. With the + /// commit first, exactly one path ever delivers an initiator-bound + /// terminal for an admission — the path that removed the entry — so the + /// wire and the event stream cannot diverge. The cost is deliberate: a + /// result whose commit loses the race is discarded, exactly as a result + /// arriving a moment after the cancel would have been dropped at the + /// peek. The "first terminal frame wins" client contract (see the ADR) + /// remains as defence in depth, but the CP itself no longer produces + /// competing terminal frames for one admission. /// - /// Nothing outside the commit's exact-match window is touched, so the - /// peek-send window cannot corrupt CP state: a concurrent cancel, sweep, - /// or disconnect that already ended the delegation leaves this frame with - /// `Delivered { committed: false }`, and an id re-admitted in the window - /// keeps its own live entry and capacity. + /// A delivery failure after the commit does not reopen the delegation: + /// an initiator whose bounded queue refuses the frame is disconnected by + /// contract ([`CompleteOutcome::Completed::stalled_initiator`]), and a + /// disconnected initiator loses results — the same fate as one that died + /// a moment earlier. Its teardown finds no entry and synthesizes nothing. /// - /// What the window *can* still produce is more than one terminal frame on - /// the wire for one `delegation_id` — a `completed` result racing the - /// sweep's synthesized `timeout`, or two duplicate results both passing - /// the peek. That is resolved by contract, not by CP-side suppression: - /// initiators MUST treat the FIRST terminal frame for a given **admission - /// token** as authoritative and ignore later ones for that token (see - /// "first terminal frame wins" in the ADR's v1 contract amendments). Every - /// initiator-bound terminal frame carries the token of the admission it - /// ends — CP-synthesized `timeout` and `target_disconnected` included — so - /// a late frame for a superseded admission can never mask the live one. + /// The token check in phase 1 extends admission exactness past the CP + /// boundary: a late result for a cancelled admission A, arriving after + /// the same `delegation_id` was re-admitted as B to the same worker, is + /// dropped instead of being delivered as B's terminal. /// /// Only the instance the delegation was routed to may complete it; a /// non-owner frame can never make the delegation momentarily invisible /// to a genuine result or to the deadline sweep. /// - /// Observers in the delegation's namespace are notified of the terminal - /// status only when THIS path authoritatively ends the delegation - /// (`Commit::Claimed`). Every other ending — cancel, sweep, initiator or - /// target disconnect, forward rollback — emits its own terminal from its - /// own removal, so the event stream carries exactly one authoritative - /// terminal per admission and it always matches CP state: a result whose - /// delivery stalls terminates through `fail_instance` and observers see - /// `delegation_cancelled`, the same outcome the initiator records. - /// Dropped and stale results emit nothing. + /// Observers are notified of the terminal status only when THIS path + /// authoritatively ends the delegation (`Commit::Claimed`). Every other + /// ending — cancel, sweep, initiator or target disconnect, forward + /// rollback — emits its own terminal from its own removal, so the event + /// stream carries exactly one authoritative terminal per admission and it + /// always matches what the initiator was sent. Dropped and stale results + /// emit nothing and deliver nothing. pub fn complete( &self, registry: &Registry, @@ -898,9 +977,9 @@ impl Router { max_result_bytes: usize, next_rpc_id: u64, ) -> CompleteOutcome { - // Phase 1 — peek: validate without removing. Removing before the - // send would make a refused send unrecoverable (silent loss of a - // computed result while the serving side is acked as delivered). + // Phase 1 — peek: validate ownership and the echoed admission token + // without removing anything, so refused/foreign/stale frames leave + // the table untouched. let namespace = match registry.get(serving_handle) { Some(i) => i.namespace, None => { @@ -963,63 +1042,25 @@ impl Router { // initiator and the lobby both see). cap_result(&mut params, max_result_bytes); - // Phase 3 — deliver. Irreversible: once the frame is queued the - // initiator will see this terminal, which is why the commit that makes - // it authoritative comes after, and why a refusal here must NOT emit a - // terminal event (`fail_instance` owns that path). - match deliver_result(registry, &entry, ¶ms, next_rpc_id) { - Deliver::Queued => {} - Deliver::InitiatorGone => return CompleteOutcome::Dropped, - Deliver::Refused => { - return CompleteOutcome::InitiatorStalled { - initiator_handle: entry.from_handle, - } - } - } - - // Phase 4 — commit. Claims ONLY the admission that was peeked; see + // Phase 3 — commit. Claims ONLY the admission that was peeked; see // `commit_completion` for why key + serving handle is not enough. - let committed = match self.commit_completion(registry, &entry) { - Commit::Claimed => { - info!( - delegation = %params.delegation_id, - status = ?params.status, - from = %entry.to_logical, - to = %entry.from_logical, - "delegation completed" - ); - // Lobby fan-out AFTER the authoritative removal: this path - // owns the entry's end, so it owns the terminal event. Every - // removal path (commit here, cancel, sweep, fail_instance, - // forward rollback) emits exactly one terminal from its own - // removal, so observers never see divergent or duplicate - // terminals for one admission, and a dropped or stale result - // emits nothing at all. - events.emit( - registry, - &entry.namespace, - CpEvent::DelegationCompleted { - delegation_id: params.delegation_id.clone(), - admission: entry.generation, - from: entry.from_logical.clone(), - to: entry.to_logical.clone(), - status: params.status.clone(), - result_excerpt: events - .excerpt_opt(&entry.namespace, params.result.as_deref()), - error: events.excerpt_opt(&entry.namespace, params.error.as_deref()), - }, - ); - true - } + // The commit precedes emission and delivery so that at most one path + // ever announces or delivers a terminal for this admission — see the + // method doc for why this order is load-bearing. + match self.commit_completion(registry, &entry) { + Commit::Claimed => {} Commit::Vanished => { - // Concurrent removal (duplicate result, cancel, sweep, or - // fail_instance): whoever removed it released the capacity. + // A concurrent cancel, sweep, disconnect, or duplicate result + // ended the delegation first. That path owns both terminals; + // this result is discarded UNDELIVERED so the initiator and + // the observer stream keep telling the same story. info!( delegation = %params.delegation_id, namespace = %entry.namespace, - "entry removed concurrently after delivery — capacity already released" + "delegation ended concurrently before this result committed \ + — result dropped, terminal owned by the concurrent path" ); - false + return CompleteOutcome::Dropped; } Commit::Superseded { generation } => { // The id was cancelled/expired and re-admitted between peek @@ -1031,12 +1072,58 @@ impl Router { namespace = %entry.namespace, peeked_generation = entry.generation, live_generation = generation, - "delegation id re-admitted between delivery and commit — live entry left untouched" + "delegation id re-admitted between peek and commit — \ + result dropped, live entry left untouched" ); - false + return CompleteOutcome::Dropped; } - }; - CompleteOutcome::Delivered { committed } + } + + info!( + delegation = %params.delegation_id, + status = ?params.status, + from = %entry.to_logical, + to = %entry.from_logical, + "delegation completed" + ); + + // Phase 4a — emit. Lobby fan-out AFTER the authoritative removal: + // this path owns the entry's end, so it owns the terminal event. + // Every removal path (commit here, cancel, sweep, fail_instance, + // forward rollback) emits exactly one terminal from its own removal, + // so observers never see divergent or duplicate terminals for one + // admission, and a dropped or stale result emits nothing at all. + events.emit( + registry, + &entry.namespace, + CpEvent::DelegationCompleted { + delegation_id: params.delegation_id.clone(), + admission: entry.generation, + from: entry.from_logical.clone(), + to: entry.to_logical.clone(), + status: params.status.clone(), + result_excerpt: events.excerpt_opt(&entry.namespace, params.result.as_deref()), + error: events.excerpt_opt(&entry.namespace, params.error.as_deref()), + }, + ); + + // Phase 4b — deliver. The delegation is already over; delivery + // failure is a fact about the initiator's connection, not about the + // delegation's outcome. + match deliver_result(registry, &entry, ¶ms, next_rpc_id) { + Deliver::Queued => CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None, + }, + Deliver::InitiatorGone => CompleteOutcome::Completed { + delivered: false, + stalled_initiator: None, + }, + Deliver::Refused => CompleteOutcome::Completed { + delivered: false, + stalled_initiator: Some(entry.from_handle), + }, + } } /// Handle `cp/cancel` from the initiator. Returns the frame to forward @@ -1190,77 +1277,91 @@ impl Router { keys.iter().filter_map(|k| g.remove(k)).collect() }; for e in entries { + // An entry removed before `delegate`'s announce/forward section + // ran was never announced to observers and never forwarded to the + // worker: emit no terminal (it would dangle without a + // `requested`) and synthesize no frames (a result would reach an + // initiator whose `cp/delegate` call is itself returning an + // error, and a cancel would name an admission the worker never + // saw). Capacity release below still applies — the reservation + // was made at insert, before the announce. if e.to_handle == handle { // Serving side died → tell the initiator. let error = format!("{} disconnected", e.to_logical); - if let Some(init) = registry.get(e.from_handle) { - let params = DelegateResultParams { - delegation_id: e.delegation_id.clone(), - // The admission this frame ends — the initiator - // correlates terminal frames on the token, not on the - // reusable id. - admission: e.generation, - status: DelegationStatus::TargetDisconnected, - result: None, - error: Some(error.clone()), - }; - let frame = JsonRpcRequest::new( - rpc_id(), - methods::DELEGATE_RESULT, - Some(serde_json::to_value(¶ms).expect("serializable")), + if e.announced { + if let Some(init) = registry.get(e.from_handle) { + let params = DelegateResultParams { + delegation_id: e.delegation_id.clone(), + // The admission this frame ends — the initiator + // correlates terminal frames on the token, not on + // the reusable id. + admission: e.generation, + status: DelegationStatus::TargetDisconnected, + result: None, + error: Some(error.clone()), + }; + let frame = JsonRpcRequest::new( + rpc_id(), + methods::DELEGATE_RESULT, + Some(serde_json::to_value(¶ms).expect("serializable")), + ); + affected.push((init, serde_json::to_string(&frame).expect("serializable"))); + } + // Emitted even when the initiator is already gone: the + // lobby must see the delegation reach a terminal state. + events.emit( + registry, + &e.namespace, + CpEvent::DelegationCompleted { + delegation_id: e.delegation_id.clone(), + admission: e.generation, + from: e.from_logical.clone(), + to: e.to_logical.clone(), + status: DelegationStatus::TargetDisconnected, + result_excerpt: None, + error: Some(events.cp_diagnostic(&error)), + }, ); - affected.push((init, serde_json::to_string(&frame).expect("serializable"))); } - // Emitted even when the initiator is already gone: the lobby - // must see the delegation reach a terminal state. - events.emit( - registry, - &e.namespace, - CpEvent::DelegationCompleted { - delegation_id: e.delegation_id.clone(), - admission: e.generation, - from: e.from_logical.clone(), - to: e.to_logical.clone(), - status: DelegationStatus::TargetDisconnected, - result_excerpt: None, - error: Some(events.cp_diagnostic(&error)), - }, - ); } else { // Initiator died → cancel downstream, free worker capacity. registry.adjust_sessions(e.to_handle, -1); let reason = format!("initiator {} disconnected", e.from_logical); - if let Some(target) = registry.get(e.to_handle) { - let params = CancelParams { - delegation_id: e.delegation_id.clone(), - // The admission this cancel ends. Built from the entry - // this loop removed, so if the id is re-admitted before - // this best-effort frame reaches the worker, the frame - // still names the admission that is over. - admission: e.generation, - reason: reason.clone(), - }; - let frame = JsonRpcRequest::new( - rpc_id(), - methods::CANCEL, - Some(serde_json::to_value(¶ms).expect("serializable")), + if e.announced { + if let Some(target) = registry.get(e.to_handle) { + let params = CancelParams { + delegation_id: e.delegation_id.clone(), + // The admission this cancel ends. Built from the + // entry this loop removed, so if the id is + // re-admitted before this best-effort frame + // reaches the worker, the frame still names the + // admission that is over. + admission: e.generation, + reason: reason.clone(), + }; + let frame = JsonRpcRequest::new( + rpc_id(), + methods::CANCEL, + Some(serde_json::to_value(¶ms).expect("serializable")), + ); + affected + .push((target, serde_json::to_string(&frame).expect("serializable"))); + } + events.emit( + registry, + &e.namespace, + CpEvent::DelegationCancelled { + delegation_id: e.delegation_id.clone(), + admission: e.generation, + from: e.from_logical.clone(), + to: e.to_logical.clone(), + by: "control-plane".to_string(), + reason: Some(events.cp_diagnostic(&reason)), + }, ); - affected.push((target, serde_json::to_string(&frame).expect("serializable"))); } - events.emit( - registry, - &e.namespace, - CpEvent::DelegationCancelled { - delegation_id: e.delegation_id.clone(), - admission: e.generation, - from: e.from_logical.clone(), - to: e.to_logical.clone(), - by: "control-plane".to_string(), - reason: Some(events.cp_diagnostic(&reason)), - }, - ); } - warn!(delegation = %e.delegation_id, handle, "in-flight delegation failed by disconnect"); + warn!(delegation = %e.delegation_id, handle, announced = e.announced, "in-flight delegation failed by disconnect"); } affected } @@ -1286,7 +1387,15 @@ impl Router { let mut frames = Vec::new(); for e in overdue { registry.adjust_sessions(e.to_handle, -1); - warn!(delegation = %e.delegation_id, deadline = %e.deadline, "delegation deadline exceeded"); + warn!(delegation = %e.delegation_id, deadline = %e.deadline, announced = e.announced, "delegation deadline exceeded"); + if !e.announced { + // Removed before `delegate`'s announce/forward section ran + // (possible only with a deadline at or before admission + // time): never announced, never forwarded — no terminal to + // emit, no frames to synthesize. Capacity was still reserved + // at insert, hence the release above. + continue; + } events.emit( registry, &e.namespace, @@ -1355,39 +1464,44 @@ impl Router { } } -/// Truncate `s` to at most `cap` bytes, keeping the head and appending a -/// marker. The marker counts against the cap: the returned value never -/// exceeds `cap` (review round-2 F5), and cuts always land on UTF-8 char -/// boundaries. Shared by result capping and observer excerpts so both use -/// one implementation. -/// Outcome of the delivery phase of a completion. Delivery is the irreversible -/// half of `complete`: the commit that makes a terminal authoritative — and the -/// lobby event that records it — happen only after `Queued`. +/// Outcome of the delivery phase of a completion. Under commit-first +/// ordering, delivery runs AFTER the commit that ended the delegation and +/// after the observer terminal was emitted: a failure here is a fact about +/// the initiator's connection, never about the delegation's outcome, and +/// must not emit a second terminal. enum Deliver { /// The frame is on the initiator's outbound queue. Queued, - /// The initiator deregistered concurrently; its own `fail_instance` pass - /// removes the entry, releases capacity and cancels the serving side, and - /// owns the terminal event. + /// The initiator deregistered before delivery. The delegation still + /// completed (the worker did the work); the dead initiator loses the + /// result exactly as it loses everything else in flight at its death. + /// Its teardown finds no in-flight entry and synthesizes nothing. InitiatorGone, - /// The initiator's bounded queue refused the frame. The entry stays in - /// flight; the caller closes that connection and its teardown fails the - /// delegation over `fail_instance`, which emits the single authoritative - /// terminal. Emitting `completed` here too would publish two contradictory - /// terminals for one admission. + /// The initiator's bounded queue refused the frame. Per the queue + /// contract it is treated as disconnected: the caller closes that + /// connection, and because the entry was already committed, the teardown + /// finds nothing to fail — no conflicting terminal can be produced. Refused, } -/// Cap an oversized result in place (keep the head — the delegation already -/// ran). The marker counts against the cap, so the final value never exceeds -/// `max_result_bytes`, and the capped value is what the initiator and the lobby -/// both see. +/// Cap an oversized result or error in place (keep the head — the delegation +/// already ran). The marker counts against the cap, so the final value never +/// exceeds `max_result_bytes`, and the capped value is what the initiator and +/// the lobby both see. `error` is agent free text from the same frame and the +/// same sender as `result`, so it gets the same bound — an uncapped error +/// would ride up to the transport frame limit while the result beside it is +/// capped. fn cap_result(params: &mut DelegateResultParams, max_result_bytes: usize) { if let Some(r) = ¶ms.result { if r.len() > max_result_bytes { params.result = Some(truncate_with_marker(r, max_result_bytes)); } } + if let Some(e) = ¶ms.error { + if e.len() > max_result_bytes { + params.error = Some(truncate_with_marker(e, max_result_bytes)); + } + } } /// Queue the terminal result on the initiator's connection. @@ -1423,6 +1537,11 @@ fn deliver_result( Deliver::Queued } +/// Truncate `s` to at most `cap` bytes, keeping the head and appending a +/// marker. The marker counts against the cap: the returned value never +/// exceeds `cap`, and cuts always land on UTF-8 char boundaries. Shared by +/// result/error capping, the server's inbound cancel-reason cap, and the +/// observer excerpt paths so all use one implementation. pub(crate) fn truncate_with_marker(s: &str, cap: usize) -> String { if s.len() <= cap { return s.to_string(); @@ -1705,7 +1824,10 @@ type = "worker" 1024, 4 ), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let frame = w.primary_rx.try_recv().expect("initiator got B's result"); assert!(frame.contains("B's genuine result")); @@ -1871,7 +1993,10 @@ type = "worker" 1024, 5 ), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let frame = w.primary_rx.try_recv().expect("initiator got B's result"); assert!(frame.contains("B's genuine result")); @@ -1930,7 +2055,10 @@ type = "worker" 1024, 5 ), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let terminal_b = w.primary_rx.try_recv().unwrap(); assert_eq!(frame_admission(&terminal_b), b.admission); @@ -2030,7 +2158,10 @@ type = "primary" 1024, 2 ), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); assert_eq!(router.inflight_count(), 0); let second = accept(go("d-2")); @@ -2136,7 +2267,10 @@ type = "primary" assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); // Delivery lands on the initiator's own queue — the frame arriving on // `primary_rx` IS the `init.handle == h_primary` assertion. @@ -2169,7 +2303,10 @@ type = "primary" assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); w.worker_rx.try_recv().unwrap(); } @@ -2264,13 +2401,14 @@ type = "primary" } #[test] - fn stalled_initiator_result_is_never_silently_lost() { - // Terminal results honor the bounded-queue contract: if the - // initiator cannot drain its queue, the entry stays in flight and - // the caller is told to treat the initiator as disconnected. The - // delegation then resolves through fail_instance (cp/cancel to the - // serving side, capacity released once) — never by silently - // dropping a computed result while acking the serving side. + fn stalled_initiator_result_commits_and_reports_the_stall() { + // Commit-first: the delegation ends when the result commits, before + // delivery is attempted. An initiator whose bounded queue refuses the + // terminal frame is disconnected by contract and loses the result — + // the same fate as one that died a moment earlier — and the caller is + // told to close its connection. The entry is already gone, so the + // subsequent teardown finds nothing to fail and synthesizes nothing: + // no second, conflicting terminal can exist. let mut w = world(); assert!(matches!( do_delegate(&w, delegate_params("d-1", "worker-1", 60)), @@ -2292,29 +2430,242 @@ type = "primary" 1024, 2 ), - CompleteOutcome::InitiatorStalled { - initiator_handle: w.h_primary + CompleteOutcome::Completed { + delivered: false, + stalled_initiator: Some(w.h_primary) } ); - assert_eq!(w.router.inflight_count(), 1, "entry must stay in flight"); + assert_eq!(w.router.inflight_count(), 0, "the commit ended the entry"); assert_eq!( w.registry.get(w.h_worker).unwrap().active_sessions, - 1, - "capacity must not be released while the delegation is unresolved" + 0, + "capacity released by the commit, exactly once" ); - // The stalled initiator is then failed (disconnect path): capacity - // is released exactly once and the serving side is told to cancel. + // The stalled initiator's teardown finds nothing: no duplicate + // capacity release, no synthesized cancel to the worker, no second + // terminal. let mut next = || 3; let frames = w .router .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); - assert_eq!(frames.len(), 1); - assert!(frames[0].1.contains("cp/cancel")); + assert!(frames.is_empty(), "nothing left for the teardown to fail"); + assert_eq!(w.registry.get(w.h_worker).unwrap().active_sessions, 0); + } + + #[test] + fn result_racing_a_concurrent_cancel_is_dropped_undelivered() { + // Review round-8 F40: the old deliver-then-commit order let a result + // reach the initiator while a concurrent cancel owned the observer + // terminal — the two audiences recorded opposite outcomes for one + // admission and no party could detect the split. Commit-first drops + // the losing result UNDELIVERED: whoever removes the entry owns both + // the initiator-bound frame and the observer terminal. This test + // drives the exact peek → concurrent cancel → commit interleaving + // through the same private phases `complete` runs. + let mut w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.worker_rx.try_recv().unwrap(); + let admission = token(&w.router, "prod", "d-1"); + + // Phase 1 as `complete` performs it: peek the live admission. + let peeked = match w + .router + .peek_for_completion("prod", "d-1", w.h_worker, admission) + { + Peek::Serving(e) => e, + _ => panic!("expected the live admission"), + }; + + // The initiator's cancel wins the window between peek and commit. + let cancel = CancelParams { + delegation_id: "d-1".into(), + admission, + reason: "changed my mind".into(), + }; + assert!(w + .router + .cancel(&w.registry, &w.events, w.h_primary, &cancel, 3) + .is_ok()); + + // The commit finds the entry gone; under commit-first this happens + // BEFORE any delivery, so the result never reaches the initiator. + assert_eq!( + w.router.commit_completion(&w.registry, &peeked), + Commit::Vanished + ); + + // The full wire path agrees: a result arriving after the cancel is + // dropped end to end. + assert_eq!( + w.router.complete( + &w.registry, + &w.events, + w.h_worker, + result_of("d-1", admission, "done"), + 1024, + 4 + ), + CompleteOutcome::Dropped + ); + + // No initiator-bound result frame; the observer stream carries the + // cancel path's terminal and never a completed. + assert!( + w.primary_rx.try_recv().is_err(), + "the losing result must not be delivered" + ); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 2, "requested + the cancel's terminal only"); + assert_eq!(ev[0]["event"], "delegation_requested"); + assert_eq!(ev[1]["event"], "delegation_cancelled"); + assert_eq!(ev[1]["admission"], admission); + assert_eq!( + w.registry.get(w.h_worker).unwrap().active_sessions, + 0, + "capacity released exactly once, by the cancel" + ); + } + + #[test] + fn teardown_of_an_unannounced_entry_emits_nothing_and_sends_nothing() { + // Review round-8 F39: a teardown can remove an entry in the window + // between the admission insert and `delegate`'s announce/forward + // critical section. Such an admission was never announced to + // observers and never forwarded to the worker, so the teardown must + // emit no observer terminal (it would dangle without a `requested`) + // and synthesize no wire frames (a cancel would name an admission + // the worker never saw) — only release the capacity reserved at + // insert. The announce/forward section then finds the entry gone and + // forwards nothing, so the worker can never run cancelled work. + let w = world(); + let mut lobby = observe(&w, "prod"); + + // Construct the in-between state directly: entry inserted, capacity + // reserved, `announced` still false — exactly what a concurrent + // teardown can observe. + w.registry.adjust_sessions(w.h_worker, 1); + let entry = InFlight { + namespace: "prod".into(), + delegation_id: "d-race".into(), + from_logical: "prod/koudu".into(), + from_handle: w.h_primary, + to_logical: "prod/worker-1".into(), + to_handle: w.h_worker, + deadline: Utc::now() + Duration::seconds(60), + chain: vec!["prod/koudu".into()], + generation: 1, + announced: false, + }; + w.router + .inflight + .lock() + .insert(DelegationKey::new("prod", "d-race"), entry); + + // Initiator teardown wins the race. + let mut next = || 11; + let frames = w + .router + .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); + + assert!( + frames.is_empty(), + "no cp/cancel for a never-forwarded admission" + ); + assert!( + events_of(&mut lobby).is_empty(), + "no observer event for a never-announced admission" + ); + assert_eq!(w.router.inflight_count(), 0, "entry removed"); + assert_eq!( + w.registry.get(w.h_worker).unwrap().active_sessions, + 0, + "insert-time capacity reservation released exactly once" + ); + } + + #[test] + fn sweep_of_an_unannounced_entry_releases_capacity_and_stays_silent() { + // Same F39 gating on the deadline-sweep removal path: an entry the + // sweep reaps before it was announced (a deadline at or before + // admission time) releases its capacity but emits no terminal and + // synthesizes no frames. + let w = world(); + let mut lobby = observe(&w, "prod"); + w.registry.adjust_sessions(w.h_worker, 1); + let entry = InFlight { + namespace: "prod".into(), + delegation_id: "d-expired".into(), + from_logical: "prod/koudu".into(), + from_handle: w.h_primary, + to_logical: "prod/worker-1".into(), + to_handle: w.h_worker, + deadline: Utc::now() - Duration::seconds(1), + chain: vec!["prod/koudu".into()], + generation: 1, + announced: false, + }; + w.router + .inflight + .lock() + .insert(DelegationKey::new("prod", "d-expired"), entry); + + let mut next = || 13; + let frames = w + .router + .sweep_deadlines(&w.registry, &w.events, Utc::now(), &mut next); + + assert!(frames.is_empty(), "no synthesized frames"); + assert!(events_of(&mut lobby).is_empty(), "no observer events"); assert_eq!(w.router.inflight_count(), 0); assert_eq!(w.registry.get(w.h_worker).unwrap().active_sessions, 0); } + #[test] + fn oversized_error_truncated_like_result() { + // Carried review finding (R6-F14): `error` is agent free text from + // the same frame and the same sender as `result`; leaving it uncapped + // let it ride to the initiator bounded only by the transport frame + // limit while the result beside it was capped. Both now share + // `max_result_bytes`. + let mut w = world(); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.worker_rx.try_recv().unwrap(); + let result = DelegateResultParams { + delegation_id: "d-1".into(), + admission: token(&w.router, "prod", "d-1"), + status: DelegationStatus::Failed, + result: None, + error: Some("e".repeat(200)), + }; + let cap = 96usize; + assert_eq!( + w.router + .complete(&w.registry, &w.events, w.h_worker, result, cap, 2), + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } + ); + let frame = w.primary_rx.try_recv().unwrap(); + let v: serde_json::Value = serde_json::from_str(&frame).unwrap(); + let out = v["params"]["error"].as_str().unwrap(); + assert!(out.contains("truncated by control plane")); + assert!( + out.len() <= cap, + "marker must count against the cap: {} > {}", + out.len(), + cap + ); + } + #[test] fn duplicate_delegation_id_rejected() { let w = world(); @@ -2448,7 +2799,10 @@ type = "primary" assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result, cap, 2), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let frame = w.primary_rx.try_recv().unwrap(); let v: serde_json::Value = serde_json::from_str(&frame).unwrap(); @@ -2477,7 +2831,10 @@ type = "primary" assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result2, 8, 3), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let frame2 = w.primary_rx.try_recv().unwrap(); let v2: serde_json::Value = serde_json::from_str(&frame2).unwrap(); @@ -3209,7 +3566,10 @@ allow_worker_initiation = true 1024, 3, ), - CompleteOutcome::Delivered { committed: true }, + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + }, "genuine result must be delivered, never dropped" ); let frame = w.primary_rx.try_recv().unwrap(); @@ -3261,7 +3621,10 @@ allow_worker_initiation = true result_of("d-1", tok, "spoofed"), 1024, 2, - ) == CompleteOutcome::Delivered { committed: true } + ) == CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None, + } }); gate.wait(); let genuine = w.router.complete( @@ -3271,7 +3634,10 @@ allow_worker_initiation = true result_of("d-1", tok, "genuine"), 1024, 3, - ) == CompleteOutcome::Delivered { committed: true }; + ) == CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None, + }; (spoof.join().unwrap(), genuine) }); assert!(!spoofed, "a non-owner must never complete a delegation"); @@ -3498,7 +3864,10 @@ allow_worker_initiation = true 1024, 12 ), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); // Landing on `dev_init_rx` is what "routed to hp_dev" means now that // delivery happens inside `complete`. @@ -3522,7 +3891,10 @@ allow_worker_initiation = true 1024, 13 ), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let frame = prod_init_rx.try_recv().unwrap(); assert!(frame.contains("prod-done")); @@ -3761,7 +4133,10 @@ allow_worker_initiation = true 1024, 7 ), - CompleteOutcome::Delivered { committed: true }, + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + }, "{how:?}" ); let frame = w.primary_rx.try_recv().expect("initiator got the result"); @@ -4029,7 +4404,10 @@ type = "primary" assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let ev = events_of(&mut lobby); @@ -4045,12 +4423,12 @@ type = "primary" #[test] fn completed_event_emitted_even_when_initiator_is_gone() { - // Renamed contract (review round-7 F4/F5): an undelivered result is - // NOT an authoritative terminal. When the initiator is gone the - // result is dropped with NO observer event; the one terminal the - // lobby sees comes from the initiator's teardown (`fail_instance`) - // and matches what actually happened: the delegation was cancelled, - // its result never delivered. + // Commit-first contract (review round-8 F40): the worker finished and + // its result committed, so the delegation truthfully completed — the + // observer sees the worker's terminal even though the initiator died + // before delivery and loses the result. The commit removed the entry, + // so the initiator's teardown finds nothing and can never publish a + // second, conflicting terminal. let w = world(); let mut lobby = observe(&w, "prod"); assert!(matches!( @@ -4069,36 +4447,40 @@ type = "primary" assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), - CompleteOutcome::Dropped, - "nobody left to receive the result" + CompleteOutcome::Completed { + delivered: false, + stalled_initiator: None + }, + "committed; undeliverable because the initiator is gone" ); let ev = events_of(&mut lobby); - assert_eq!( - ev.len(), - 1, - "a dropped result must not emit a terminal event" - ); + assert_eq!(ev.len(), 2, "requested + the commit's terminal"); assert_eq!(ev[0]["event"], "delegation_requested"); assert_eq!(ev[0]["admission"], admission); + assert_eq!(ev[1]["event"], "delegation_completed"); + assert_eq!(ev[1]["admission"], admission); + assert_eq!(ev[1]["status"], "failed"); - // The teardown path owns the end of the delegation and its terminal. + // The initiator's teardown finds nothing: no second terminal. let mut next = || 7; - w.router + let frames = w + .router .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); + assert!(frames.is_empty(), "the commit already ended the delegation"); let ev = events_of(&mut lobby); - assert_eq!(ev.len(), 1, "exactly one authoritative terminal"); - assert_eq!(ev[0]["event"], "delegation_cancelled"); - assert_eq!(ev[0]["admission"], admission); - assert_eq!(ev[0]["by"], "control-plane"); + assert!(ev.is_empty(), "exactly one authoritative terminal"); } #[test] - fn stalled_initiator_produces_single_cancelled_terminal_for_observers() { - // Review round-7 F4: previously `complete()` emitted - // `delegation_completed` before the initiator try_send, so a stalled - // initiator produced completed-then-cancelled — opposite outcomes - // for the two audiences. Now the observer records exactly what the - // initiator records: one cancelled terminal from `fail_instance`. + fn stalled_initiator_produces_single_completed_terminal_for_observers() { + // Review round-8 F40 follow-through: under commit-first the result + // commits BEFORE delivery, so a stalled initiator no longer flips the + // outcome. The observer records the truth — the worker completed the + // delegation — as the single terminal; the initiator, which received + // nothing (its queue refused the frame), is disconnected and its + // teardown finds no entry, so no cancelled terminal can follow. The + // two audiences can no longer record opposite outcomes: one saw + // `completed`, the other saw nothing at all. let mut w = world(); let mut lobby = observe(&w, "prod"); assert!(matches!( @@ -4120,29 +4502,25 @@ type = "primary" 1024, 2 ), - CompleteOutcome::InitiatorStalled { - initiator_handle: w.h_primary + CompleteOutcome::Completed { + delivered: false, + stalled_initiator: Some(w.h_primary) } ); let ev = events_of(&mut lobby); - assert_eq!( - ev.len(), - 1, - "no terminal while the delegation is unresolved" - ); + assert_eq!(ev.len(), 2, "requested + the commit's single terminal"); assert_eq!(ev[0]["event"], "delegation_requested"); + assert_eq!(ev[1]["event"], "delegation_completed"); + assert_eq!(ev[1]["admission"], admission); let mut next = || 9; w.router .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); let ev = events_of(&mut lobby); - assert_eq!( - ev.len(), - 1, - "one authoritative terminal, no completed+cancelled pair" + assert!( + ev.is_empty(), + "the teardown found nothing — no completed+cancelled pair" ); - assert_eq!(ev[0]["event"], "delegation_cancelled"); - assert_eq!(ev[0]["admission"], admission); } #[test] @@ -4272,7 +4650,10 @@ metadata_only = true 1024, 2 ), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let ev = events_of(&mut lobby); @@ -4435,7 +4816,10 @@ metadata_only = true assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); let ev = events_of(&mut lobby); assert_eq!(ev.len(), 2); @@ -4489,7 +4873,10 @@ metadata_only = true assert_eq!( w.router .complete(&w.registry, &w.events, w.h_worker, result, 1024, 2), - CompleteOutcome::Delivered { committed: true } + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } ); assert_eq!(w.router.inflight_count(), 0); drop(lobby_rx); diff --git a/crates/openab-cp/src/server.rs b/crates/openab-cp/src/server.rs index cf7a46be3..9418cc6e8 100644 --- a/crates/openab-cp/src/server.rs +++ b/crates/openab-cp/src/server.rs @@ -809,45 +809,41 @@ fn handle_frame(state: &Arc, handle: u64, text: &str) -> Option { - // The initiator cannot drain its bounded queue: per the - // queue contract it is treated as disconnected, never - // silently skipped. Its teardown fails the delegation - // over the fail_instance path (capacity released once, - // cp/cancel to this serving runtime). Do NOT ack the - // result as delivered — the serving side must know its - // result did not reach the initiator. - state - .registry - .signal_shutdown(initiator_handle, REASON_BACKPRESSURE); - let resp = JsonRpcErrorResponse::new( - rpc_id, - ErrorObject::new( - codes::TARGET_DISCONNECTED, - "initiator cannot receive the result; the delegation will be cancelled", - ), - ); - Some(serde_json::to_string(&resp).expect("serializable")) - } - // The result reached the initiator, which is what the serving - // side is being acked for. Whether THIS frame also committed - // the state transition is a CP-internal matter: a concurrent - // cancel/sweep/disconnect may have ended the delegation - // first, and the initiator resolves competing terminal frames - // by "first one wins" (see the ADR wire contract). - CompleteOutcome::Delivered { committed } => { - if !committed { + CompleteOutcome::Completed { + delivered, + stalled_initiator, + } => { + if let Some(initiator_handle) = stalled_initiator { + // The initiator cannot drain its bounded queue: per + // the queue contract it is treated as disconnected, + // never silently skipped. The delegation itself + // already committed (entry removed, capacity + // released, terminal emitted), so the teardown finds + // nothing to fail and synthesizes nothing. + state + .registry + .signal_shutdown(initiator_handle, REASON_BACKPRESSURE); + } + if !delivered { info!( handle, - "terminal result delivered, but the delegation had already been \ - ended (or its id re-admitted) — no state change" + "delegation completed and committed, but the terminal \ + frame did not reach the initiator (gone or stalled)" ); } + // The commit is what the serving side is acked for: its + // work is done and the delegation is over. Whether the + // initiator's connection survived long enough to receive + // the frame is a CP-internal matter, and the ack stays + // byte-identical to the dropped case so the reply is + // never an oracle for initiator liveness. let resp = JsonRpcResponse::new(rpc_id, serde_json::json!({"ok": true})); Some(serde_json::to_string(&resp).expect("serializable")) } - // Dropped as unknown/foreign; each case is logged in the - // router (late results after a CP restart are expected). + // Dropped as unknown/foreign/stale, or a concurrent path + // (cancel, sweep, disconnect) ended the delegation first and + // owns its terminals; each case is logged in the router (late + // results after a CP restart are expected). CompleteOutcome::Dropped => { let resp = JsonRpcResponse::new(rpc_id, serde_json::json!({"ok": true})); Some(serde_json::to_string(&resp).expect("serializable")) @@ -1421,11 +1417,13 @@ mod tests { } #[tokio::test] - async fn stalled_initiator_is_disconnected_and_serving_side_not_falsely_acked() { - // The bounded-queue contract for the one frame that matters most: - // when the initiator's queue refuses the terminal result, the - // serving side must NOT receive `ok: true`, and the initiator must - // be closed (treated as disconnected) rather than silently skipped. + async fn stalled_initiator_is_disconnected_and_result_still_commits() { + // The bounded-queue contract for the terminal result under + // commit-first: the commit ends the delegation before delivery, so + // the serving side is acked for the commit (`ok: true`, its work is + // done), the stalled initiator is closed (treated as disconnected) + // rather than silently skipped, and its teardown finds nothing — + // capacity was already released exactly once by the commit. let state = state_with(""); // Initiator whose outbound byte budget one filler frame exhausts — @@ -1503,29 +1501,28 @@ mod tests { .to_string(); let reply: serde_json::Value = serde_json::from_str(&handle_frame(&state, h_w, &res).expect("answered")).unwrap(); - assert_eq!( - reply["error"]["code"], - codes::TARGET_DISCONNECTED, - "the serving side must not be acked as delivered" + assert!( + reply.get("error").is_none(), + "the commit ended the delegation; the serving side is acked for it" ); - assert_eq!(reply["id"], 2, "the error must correlate with the request"); + assert_eq!(reply["result"]["ok"], true); + assert_eq!(reply["id"], 2, "the ack must correlate with the request"); // The initiator is told to close, with the backpressure reason. observer.changed().await.unwrap(); assert_eq!(*observer.borrow(), Some(REASON_BACKPRESSURE)); - // The delegation is still in flight: teardown of the stalled - // initiator resolves it through fail_instance (capacity released - // once, cp/cancel to the serving runtime). - assert_eq!(state.router.inflight_count(), 1); + // The commit already ended the delegation and released capacity: + // teardown of the stalled initiator finds nothing to fail, so no + // second terminal and no double release can occur. + assert_eq!(state.router.inflight_count(), 0); + assert_eq!(state.registry.get(h_w).unwrap().active_sessions, 0); let mut next = || 9; let frames = state .router .fail_instance(&state.registry, &state.events, h_i, &mut next); - assert_eq!(frames.len(), 1); - assert!(frames[0].1.contains("cp/cancel")); + assert!(frames.is_empty()); assert_eq!(state.registry.get(h_w).unwrap().active_sessions, 0); - assert_eq!(state.router.inflight_count(), 0); } // --- observer / lobby wiring (Phase 1) --- From 832670ec05fe559d3c32ced4535e01c88e4e5d1d Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:00:07 +0000 Subject: [PATCH 7/9] fix(review): round-8 docs and type-consistency findings - Document the observer wire contract for external implementers (docs/control-plane.md "Observer surface"): cp/event envelope and the five event kinds, the per-namespace seq client contract (baseline, gap, restart), correlation on (namespace, delegation_id, admission), lifecycle ordering guarantees, terminal asymmetry (completed-with-status vs cancelled-with-by, the control-plane sentinel), best-effort delivery, metadata_only redaction scope, and cp/list_agents including its roster-only recovery scope (carried R4-F14 / R6-F16). - Refresh the stale status block: this slice ships the observer surface, not "streaming lands later" (round-8 docs finding). - Record in the ADR that Phase 1 of the observer endpoint shipped, with its accepted operational limits (best-effort delivery, non-durable seq, no replay), and align the first-terminal-wins sections in the ADR and the client doc with commit-gated delivery (at most one initiator-bound terminal per admission; rule retained as defence in depth). - metadata_only config docs now state the full suppression scope, including cancel reasons and worker-reported error text, and that CP-synthesized diagnostics survive the knob (F43); document DelegationCompleted.error's status-dependent redaction (F44). - Type the three CpEvent admission fields as AdmissionToken to match the wire structs (F50). cargo test -p openab-cp: 141 passed. clippy -D warnings clean; rustfmt clean. --- crates/openab-cp/cp.toml.example | 11 ++-- crates/openab-cp/src/config.rs | 12 ++-- crates/openab-cp/src/proto.rs | 13 ++++- docs/adr/agent-control-plane.md | 39 ++++++++----- docs/control-plane.md | 98 +++++++++++++++++++++++++++++--- 5 files changed, 139 insertions(+), 34 deletions(-) diff --git a/crates/openab-cp/cp.toml.example b/crates/openab-cp/cp.toml.example index a0d735473..d4a7f4697 100644 --- a/crates/openab-cp/cp.toml.example +++ b/crates/openab-cp/cp.toml.example @@ -105,8 +105,11 @@ type = "observer" [namespaces.prod] max_depth = 1 allow_worker_initiation = false -# Withhold prompt/result bodies from cp/event: observers still see every -# event with full attribution (from/to/chain), timing, and status, but no -# payload excerpts. Default false (excerpts included, bounded by -# max_event_excerpt_bytes). +# Withhold agent-supplied content from cp/event: prompt/result excerpts, +# worker-reported error text, and initiator cancel reasons are all +# suppressed. Observers still see every event with full attribution +# (from/to/chain), timing, and status, and CP-synthesized diagnostics +# (timeout / disconnect reasons) remain visible by design — the stream is +# metadata-complete but content-free. Default false (excerpts included, +# bounded by max_event_excerpt_bytes). # metadata_only = true diff --git a/crates/openab-cp/src/config.rs b/crates/openab-cp/src/config.rs index 672e9c39f..83c271052 100644 --- a/crates/openab-cp/src/config.rs +++ b/crates/openab-cp/src/config.rs @@ -218,10 +218,14 @@ pub struct NamespacePolicy { /// Whether workers may initiate delegations (depth still applies). #[serde(default)] pub allow_worker_initiation: bool, - /// Withhold prompt/result bodies from `cp/event`: observers still see - /// every event, with attribution, timing, and status, but no payload - /// excerpts. Defaults to false (excerpts included, bounded by - /// `max_event_excerpt_bytes`) — the pre-existing behavior. + /// Withhold agent-supplied content from `cp/event`: prompt excerpts, + /// result excerpts, worker-reported error text, and initiator-supplied + /// cancel reasons are all suppressed (their keys absent). Observers still + /// see every event with full attribution (`from`/`to`/`chain`), timing, + /// status, and CP-synthesized diagnostics — timeout and disconnect + /// reasons are metadata the CP composed, not agent content, so they + /// survive this knob by design. Defaults to false (excerpts included, + /// bounded by `max_event_excerpt_bytes`) — the pre-existing behavior. #[serde(default)] pub metadata_only: bool, } diff --git a/crates/openab-cp/src/proto.rs b/crates/openab-cp/src/proto.rs index b9b733431..c7f3ad163 100644 --- a/crates/openab-cp/src/proto.rs +++ b/crates/openab-cp/src/proto.rs @@ -540,7 +540,7 @@ pub enum CpEvent { /// `(namespace, delegation_id, admission)` — the token is what ties a /// terminal event to the exact admission it ends, mirroring the wire /// frames (ack/result/cancel), which all carry it. - admission: u64, + admission: AdmissionToken, from: String, to: String, /// Absent when the namespace is `metadata_only`. @@ -555,12 +555,19 @@ pub enum CpEvent { /// `(namespace, delegation_id, admission)`, never on the reusable id /// alone. First terminal event for a given admission wins; later ones /// for that admission are duplicates. - admission: u64, + admission: AdmissionToken, from: String, to: String, status: DelegationStatus, #[serde(skip_serializing_if = "Option::is_none")] result_excerpt: Option, + /// Bounded excerpt of the terminal error text, when there is one. + /// Its `metadata_only` behavior depends on who wrote it: a + /// worker-reported error (`failed` results) is agent content and is + /// suppressed (key absent) exactly like `result_excerpt`, while a + /// CP-synthesized diagnostic (`timeout`, `target_disconnected`) is + /// metadata the CP composed and survives the knob — mirroring + /// `DelegationCancelled::reason`. #[serde(skip_serializing_if = "Option::is_none")] error: Option, }, @@ -570,7 +577,7 @@ pub enum CpEvent { /// `(namespace, delegation_id, admission)`, never on the reusable id /// alone. First terminal event for a given admission wins; later ones /// for that admission are duplicates. - admission: u64, + admission: AdmissionToken, /// Initiator of the cancelled delegation (`namespace/name`) — an /// observer that missed `delegation_requested` still gets full /// attribution. diff --git a/docs/adr/agent-control-plane.md b/docs/adr/agent-control-plane.md index 6efbbf146..f98c429e1 100644 --- a/docs/adr/agent-control-plane.md +++ b/docs/adr/agent-control-plane.md @@ -484,18 +484,21 @@ recovery semantics: at this point in the stack — every serving runtime learns the token from the forwarded `cp/delegate` — so the migration is mechanical, but it is not silent and it is not optional. -- **First terminal frame per admission wins.** More than one terminal frame may - reach an initiator for one admission: a `completed` result can race the - deadline sweep's synthesized `timeout`, and duplicate results are possible in - the window between delivery and commit. **Initiators MUST treat the first - terminal frame (`completed`, `failed`, `timeout`, `target_disconnected`) for - a given `admission` token as authoritative and ignore every later terminal +- **First terminal frame per admission wins.** Delivery is commit-gated: + only the one path that authoritatively ended an admission (completion + commit, cancel, deadline sweep, disconnect teardown) may put an + initiator-bound terminal frame on the wire, and a result whose commit + loses the race is discarded undelivered — so under one CP process an + initiator receives at most one terminal frame per admission, and the + observer event stream records the same outcome. The rule is retained as + client-side defence in depth (frames straddling a CP restart, future + multi-instance deployments): **initiators MUST treat the first terminal + frame (`completed`, `failed`, `timeout`, `target_disconnected`) for a + given `admission` token as authoritative and ignore every later terminal frame for that token.** Correlation is per admission, not per `delegation_id`: keyed on the reusable id, a late frame for a superseded - admission would permanently mask the live admission's genuine terminal frame. - The CP does not suppress the later frames: doing so would require per-id - terminal state that a CP with no durable state deliberately does not keep. - CP-side state is unaffected either way — the token rule above makes the + admission would permanently mask the live admission's genuine terminal + frame. CP-side state is exact either way — the token rule above makes the commit exact, so exactly one path ever releases the capacity. - **Global admission and memory bounds.** Beyond per-frame and per-connection limits the CP bounds its own aggregate state: @@ -760,11 +763,17 @@ of scope for v1. runtimes will stream `session/update`-style chunks back through the CP. Rationale: streaming is the observability substrate, not a feature — it restores the free human visibility that Discord-mediated collaboration - provides today. It enables a read-only observer endpoint on the CP - (e.g. `wss://cp/.../observe?ns=prod`; separate read-only credential - class, namespace-scoped) so a human can tail all delegation traffic - across the fleet from one terminal. v1 ships final-result-only; the - stream frame shape is reserved in the wire contract. + provides today. *Phase 1 of the observer surface has since shipped + (PR #1470): the read-only `observer` identity type, the `cp/event` + lifecycle notification stream (per-namespace `seq`, admission-token + correlation, `metadata_only` redaction), and `cp/list_agents` — wire + contract documented in `docs/control-plane.md` ("Observer surface"). + Accepted operational limits of that slice: delivery to observers is + best-effort (bounded queues, `seq`-gap detection), `seq` is not durable + across CP restarts, and there is no event replay — a lobby needing + history must retain its own.* Intermediate `session_*` relay streaming + remains future scope. v1 of the *delegation wire contract* ships + final-result-only; the stream frame shape is reserved. 2. **CP high availability** — single instance + fast re-registration is acceptable for v1 (restart semantics are now defined in §4); is active/standby needed before multi-tenant use? diff --git a/docs/control-plane.md b/docs/control-plane.md index c55b7600f..fb9d2b6e6 100644 --- a/docs/control-plane.md +++ b/docs/control-plane.md @@ -5,11 +5,14 @@ WebSocket JSON-RPC, so agents delegate work to each other without round-tripping through a chat platform. Design and wire contract: [ADR: Agent Control Plane](adr/agent-control-plane.md). -> **Status: PR 1/4 of the control-plane stack.** This slice ships the CP -> server binary (registry, policy, router, wire protocol). The OAB-runtime +> **Status: PR 2/4 of the control-plane stack.** PR 1/4 shipped the CP +> server binary (registry, policy, router, wire protocol); this slice adds +> the observer/lobby surface — the read-only `observer` agent type, the +> `cp/event` notification stream, and `cp/list_agents` (see +> [Observer surface](#observer-surface-lobby) below). The OAB-runtime > client (`[control_plane]` config + registration), the MCP facade/CLI, and -> streaming land in the follow-up slices — until then nothing connects to -> this server in a stock deployment, and there is no packaged container +> client relay land in the follow-up slices — until then nothing connects +> to this server in a stock deployment, and there is no packaged container > image yet. ## Run @@ -98,10 +101,13 @@ issue #1474). after upgrading the CP; there is no compatible optional spelling, because an absent token would be the wildcard the field exists to remove. Root delegations are unaffected. -- **The first terminal frame for an `admission` token wins.** A `completed` - result can race the CP's synthesized `timeout`, so an initiator may receive - more than one terminal frame for the same admission. Treat the first as - authoritative and ignore later ones; the CP does not suppress them. +- **The first terminal frame for an `admission` token wins.** The CP delivers + a terminal frame only from the one path that authoritatively ended the + admission (completion commit, cancel, deadline sweep, or disconnect + teardown), so under one CP process an initiator should see exactly one + terminal per admission. Keep the rule anyway, as defence in depth (frames + straddling a CP restart, future multi-instance deployments): treat the + first terminal frame as authoritative and ignore later ones for that token. Correlate on `admission`, not on `delegation_id`: the id is yours to reuse (cancel-then-retry is legal), and a late frame for the cancelled admission would otherwise mask the retry's genuine result. Every terminal frame carries @@ -114,3 +120,79 @@ issue #1474). The ack's `effective_max_delegated_sessions` is the value that counts. - After a lease expires or the CP restarts, in-flight delegations are gone: initiators reconcile against their own deadlines and re-delegate. + +## Observer surface (lobby) + +A third identity type joins `primary`/`worker`: **`observer`** — a read-only +lobby client, authenticated by the same per-key identity binding +(`type = "observer"` on the `[[agents]]` entry). Observers register via the +same `cp/register` first-frame rule and hold a lease like any agent, but they +are read-only by construction: never selectable as a delegation target, and +unconditionally refused as an initiator — at the policy layer, at target +selection, and by an up-front method guard. There is no configuration that +relaxes this. + +### `cp/event` notifications + +The CP pushes JSON-RPC **notifications** (method `cp/event`, no `id`) to +every observer in the event's namespace. Envelope: + +```json +{"jsonrpc":"2.0","method":"cp/event","params":{ + "seq": 7, "ts": "2026-08-14T20:00:00Z", "namespace": "prod", + "event": "delegation_requested", "...": "event-specific fields" +}} +``` + +Event kinds and their fields: + +| `event` | Fields | +|---------|--------| +| `agent_registered` | `agent`, `type`, `instance_id`, `labels` | +| `agent_deregistered` | `agent`, `instance_id`, `reason` (`disconnect` / `lease_expired`) | +| `delegation_requested` | `delegation_id`, `admission`, `from`, `to`, `prompt_excerpt`?, `deadline`, `chain` | +| `delegation_completed` | `delegation_id`, `admission`, `from`, `to`, `status`, `result_excerpt`?, `error`? | +| `delegation_cancelled` | `delegation_id`, `admission`, `from`, `to`, `by`, `reason`? | + +Client contract: + +- **Sequence numbers.** `seq` is per-namespace, monotonic, and dense: your + first received frame sets your baseline, and a gap means frames were + dropped for you (saturated queue) — resync your roster via + `cp/list_agents`. A `seq` regression means the CP restarted: treat it as a + full resync. `seq` is not durable across restarts. +- **Correlate on `(namespace, delegation_id, admission)`.** A delegation id + is legally reusable (cancel-then-retry); the `admission` token is what ties + a terminal event to the exact admission it ends, mirroring the wire frames. +- **Lifecycle ordering.** `delegation_requested` is published before the + forward reaches the worker, and a terminal event is published only by the + path that authoritatively ended the admission — so you never see a terminal + before its `requested`, never more than one terminal per admission, and the + terminal you see is the same outcome the initiator was sent. An + `agent_deregistered` without a matching `agent_registered` is possible + (registration ack failure); treat removal of an unknown agent as a no-op. +- **Terminal asymmetry.** Completion-shaped endings (`completed`, `failed`, + `timeout`, `target_disconnected`) arrive as `delegation_completed` with a + `status`; cancellations (initiator cancel, initiator disconnect) arrive as + `delegation_cancelled` with `by` — the initiator's logical id, or the + literal `"control-plane"` for CP-synthesized cancellations. +- **Best-effort delivery.** Fan-out uses the same bounded per-connection + queue as everything else: a lobby that cannot keep up loses frames and + detects it via the `seq` gap. Observers can never slow the delegation path. +- **Content redaction.** Prompt/result excerpts are bounded by + `max_event_excerpt_bytes` (default 4 KiB). In a `metadata_only = true` + namespace, agent-supplied content (prompt/result excerpts, worker-reported + error text, initiator cancel reasons) is omitted entirely, while + CP-synthesized diagnostics (timeout/disconnect reasons) remain — the stream + stays metadata-complete but content-free. + +### `cp/list_agents` + +Any registered client (observers included) may call `cp/list_agents` (empty +params). It returns the roster of the **caller's own namespace** — name, +type, `instance_id`, labels, and load (`active_sessions` / +`max_delegated_sessions`) per instance; the scope comes from the +authenticated registration, never from the frame. This is the lobby's roster +view and the resync path after a `seq` gap. Note the recovery scope: the +snapshot restores the roster, not missed delegation lifecycle events — a +lobby that needs delegation history must retain its own. From 47016a90e60fb65482641bf1d19b7ac98cb44efe Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Sat, 15 Aug 2026 03:24:35 +0000 Subject: [PATCH 8/9] fix(review): bound observer fan-out, de-panic the teardown path (round-10 yellows) F51 short-term set (landed together, per review consensus): - max_observers_per_namespace (default 16): the per-namespace observer ceiling is enforced atomically at registration (count + insert under one registry write-lock acquisition; refusal is SATURATED with the cap named). Observer fan-out does bounded per-observer work inside the delegation path's in-flight critical section, so the population is now a configured latency budget instead of an operational hope. - max_event_excerpt_bytes validated to 1..=65536: zero silently emptied every excerpt (R4-F17), an oversized cap multiplies work on the delegation path. - The prompt excerpt (a scan of up to max_prompt_bytes of client input) is computed BEFORE the announce/forward critical section; the section now performs only the generation re-check, the announce flag, one bounded emission, and non-blocking sends. - Observer frame drops now log at warn (systemic lobby saturation was invisible at default log levels; R6-F19 facet). - docs/control-plane.md states the fan-out latency tradeoff honestly instead of "observers can never slow the delegation path" (F51 facet e / F56). F62 - teardown must not be able to panic: - fail_instance, sweep_deadlines, and EventHub::emit are reachable from RegistrationGuard's Drop (possibly already unwinding, where a second panic aborts the process) and from the lease sweeper. All frame/event serialization on those paths is now fail-soft: a serialization error drops the frame with an error log instead of panicking (synthesized_frame helper; emit returns on error). RegistrationGuard's must-not-panic doc now matches the implementation, and the panic-teardown regression test attaches an observer so the unwind exercises the full teardown emit surface. Also from round 10: - F55: lifecycle-ordering docs now say the observer records the outcome the CP committed, with the refused/gone-initiator caveat (control-plane.md + ADR). - F57: the stale drop(admission) comment no longer claims fan-out is outside the critical section. - F58: commit_completion doc describes its step functionally instead of a wrong phase number. - F59: cap_result renamed cap_payload (it bounds result AND error). Tests (141 -> 148): concurrent double-complete yields exactly one claim (F52, completes R4-F34); target-side unannounced teardown is silent (F53 symmetric case); dropped/stale results emit nothing with an observer attached and the stalled initiator provably receives no frame (F54, completes R6-F17); a saturated 16-observer crowd does not block delegation (F51 functional floor); registry observer-cap atomicity; config bound validation. cargo test -p openab-cp: 148 passed, 0 failed. clippy --all-targets -D warnings clean; rustfmt clean. --- crates/openab-cp/cp.toml.example | 9 + crates/openab-cp/src/config.rs | 69 +++++++ crates/openab-cp/src/events.rs | 34 +++- crates/openab-cp/src/registry.rs | 65 ++++++ crates/openab-cp/src/router.rs | 336 +++++++++++++++++++++++++++---- crates/openab-cp/src/server.rs | 71 ++++++- docs/adr/agent-control-plane.md | 4 +- docs/control-plane.md | 18 +- 8 files changed, 550 insertions(+), 56 deletions(-) diff --git a/crates/openab-cp/cp.toml.example b/crates/openab-cp/cp.toml.example index d4a7f4697..f4ea04ddc 100644 --- a/crates/openab-cp/cp.toml.example +++ b/crates/openab-cp/cp.toml.example @@ -76,8 +76,17 @@ default_max_delegated_sessions_cap = 16 # Prompt/result excerpts mirrored to observers in cp/event frames are # truncated to this size (the lobby is an audit surface, not a payload path). +# Validated to 1..=65536: excerpt serialization runs on the delegation path, +# so the cap is a latency budget as well as a content bound. max_event_excerpt_bytes = 4096 +# Ceiling on simultaneously registered observer connections per namespace. +# Observer fan-out does bounded per-observer work inside the delegation +# path's critical section, so the observer population is a configured +# latency budget: a registration beyond the cap is refused with SATURATED. +# Agents (primary/worker) are unaffected. +max_observers_per_namespace = 16 + [[agents]] key = "${CP_KEY_KOUDU}" # per-agent secret, never shared namespace = "prod" diff --git a/crates/openab-cp/src/config.rs b/crates/openab-cp/src/config.rs index 83c271052..a7fd7b6f9 100644 --- a/crates/openab-cp/src/config.rs +++ b/crates/openab-cp/src/config.rs @@ -95,6 +95,15 @@ pub struct CpConfig { /// delivery path: excerpts are truncated with a marker, never rejected. #[serde(default = "default_max_event_excerpt_bytes")] pub max_event_excerpt_bytes: usize, + /// Ceiling on simultaneously registered observer connections per + /// namespace. Observer fan-out does bounded per-observer work inside the + /// delegation path's in-flight critical section (see the router's lock + /// hierarchy note), so the total is deliberately capped: this knob is + /// what makes "bounded" a guarantee instead of an operational hope. A + /// registration that would exceed it is refused; agents (primary/worker) + /// are unaffected. + #[serde(default = "default_max_observers_per_namespace")] + pub max_observers_per_namespace: usize, /// Memory ceiling for ONE connection's outbound queue, in bytes. /// @@ -189,6 +198,10 @@ fn default_max_event_excerpt_bytes() -> usize { 4 * 1024 } +fn default_max_observers_per_namespace() -> usize { + 16 +} + /// Immutable identity claims bound to one auth key. #[derive(Debug, Clone, Deserialize)] pub struct AgentIdentity { @@ -314,6 +327,33 @@ impl CpConfig { if self.default_max_delegated_sessions_cap == 0 { bail!("default_max_delegated_sessions_cap must be at least 1"); } + // Zero silently reduces every excerpt to the bare truncation marker + // (a content-free lobby that LOOKS configured); an oversized cap + // multiplies the per-observer work the delegation path performs + // inside its in-flight critical section. Bound it on both sides. + if self.max_event_excerpt_bytes == 0 { + bail!( + "max_event_excerpt_bytes must be at least 1 (0 would silently \ + replace every excerpt with the bare truncation marker; use \ + [namespaces.X] metadata_only = true to suppress content)" + ); + } + if self.max_event_excerpt_bytes > 64 * 1024 { + bail!( + "max_event_excerpt_bytes ({}) exceeds the 65536-byte ceiling — \ + excerpts are serialized on the delegation path, and the lobby \ + is an audit surface, not a delivery path", + self.max_event_excerpt_bytes + ); + } + // Zero observers would refuse every lobby client while the surface + // is configured; the ceiling itself is what bounds fan-out work. + if self.max_observers_per_namespace == 0 { + bail!( + "max_observers_per_namespace must be at least 1 (omit observer \ + identities from [[agents]] to disable the lobby instead)" + ); + } // Bearer keys over cleartext TCP must never reach an untrusted // network: non-loopback binds require the explicit override. if !self.allow_insecure_bind && !is_loopback(&self.listen) { @@ -502,6 +542,35 @@ lease_expiry_secs = 30 assert!(cfg.validate().is_err()); } + #[test] + fn event_excerpt_bytes_bounded_on_both_sides() { + // Review rounds 4/10 (R4-F17 + F51): zero silently reduced every + // excerpt to the bare truncation marker, and an oversized cap + // multiplies work done on the delegation path. Both refused now. + let cfg: CpConfig = toml::from_str("max_event_excerpt_bytes = 0").unwrap(); + assert!(cfg.validate().is_err(), "zero excerpt cap must be refused"); + let cfg: CpConfig = toml::from_str("max_event_excerpt_bytes = 65537").unwrap(); + assert!(cfg.validate().is_err(), "oversized excerpt cap refused"); + let cfg: CpConfig = toml::from_str("max_event_excerpt_bytes = 65536").unwrap(); + cfg.validate().unwrap(); + // Default remains valid and unchanged. + let cfg: CpConfig = toml::from_str(base_toml()).unwrap(); + cfg.validate().unwrap(); + assert_eq!(cfg.max_event_excerpt_bytes, 4096); + } + + #[test] + fn observer_cap_defaults_and_zero_is_refused() { + // Review round-10 F51 (facet d): the observer population is a + // configured latency budget; zero would refuse every lobby client + // while the surface is configured. + let cfg: CpConfig = toml::from_str(base_toml()).unwrap(); + cfg.validate().unwrap(); + assert_eq!(cfg.max_observers_per_namespace, 16); + let cfg: CpConfig = toml::from_str("max_observers_per_namespace = 0").unwrap(); + assert!(cfg.validate().is_err()); + } + #[test] fn non_loopback_bind_requires_override() { let cfg: CpConfig = toml::from_str("listen = \"0.0.0.0:9800\"").unwrap(); diff --git a/crates/openab-cp/src/events.rs b/crates/openab-cp/src/events.rs index 113a0893e..88a77cc01 100644 --- a/crates/openab-cp/src/events.rs +++ b/crates/openab-cp/src/events.rs @@ -117,19 +117,39 @@ impl EventHub { event, }; // Serialized once per emission; registry/hub map locks are not held. - let text = serde_json::to_string(&JsonRpcNotification::new( - methods::EVENT, - serde_json::to_value(¶ms).expect("serializable"), - )) - .expect("serializable"); + // Fail SOFT on a serialization error: this function is reachable from + // connection teardown (`RegistrationGuard`'s Drop, which may already + // be unwinding — a second panic would abort the process) and from the + // lease sweeper. The lobby is an audit surface; dropping one frame + // beats killing the control plane. Practically unreachable for these + // types, but the audit path must not be able to panic by + // construction. + let text = match serde_json::to_value(¶ms) + .and_then(|v| serde_json::to_string(&JsonRpcNotification::new(methods::EVENT, v))) + { + Ok(t) => t, + Err(e) => { + tracing::error!( + namespace, + seq = params.seq, + error = %e, + "cp/event serialization failed — frame dropped (observers \ + will detect the seq gap and resync)" + ); + return; + } + }; for o in observers { // Best effort by design: a saturated lobby queue drops the frame. + // Logged at warn: systemic frame loss (a saturated lobby) must be + // visible at default log levels, not only under debug. if o.tx.try_send(text.clone()).is_err() { - tracing::debug!( + tracing::warn!( observer = %o.logical_id(), instance = %o.instance_id, seq = params.seq, - "observer queue full or closed — event frame dropped" + "observer queue full or closed — event frame dropped \ + (client resyncs via the seq gap)" ); } } diff --git a/crates/openab-cp/src/registry.rs b/crates/openab-cp/src/registry.rs index c24e67f4e..db515f068 100644 --- a/crates/openab-cp/src/registry.rs +++ b/crates/openab-cp/src/registry.rs @@ -269,6 +269,40 @@ impl Registry { handle } + /// [`Registry::register_conn`], but refused when the instance is an + /// observer and its namespace already holds `max_observers_per_namespace` + /// observer registrations. Count and insert happen under ONE write-lock + /// acquisition, so two racing observer registrations cannot both squeeze + /// under the cap. Agents (primary/worker) are never refused here. + /// + /// The cap is what makes the delegation path's fan-out work bounded by + /// configuration instead of by operational hope: observer fan-out runs + /// inside the router's in-flight critical section (see its lock + /// hierarchy note), so the number of observers is a latency budget. + pub fn register_conn_capped( + &self, + mut inst: Instance, + shutdown: ShutdownTx, + max_observers_per_namespace: usize, + ) -> Result { + let handle = self.next_handle.fetch_add(1, Ordering::Relaxed) + 1; + inst.handle = handle; + let mut g = self.inner.write(); + if inst.agent_type == AgentType::Observer { + let observers = g + .values() + .filter(|e| { + e.inst.agent_type == AgentType::Observer && e.inst.namespace == inst.namespace + }) + .count(); + if observers >= max_observers_per_namespace { + return Err(observers); + } + } + g.insert(handle, Entry { inst, shutdown }); + Ok(handle) + } + /// Register an instance with a detached shutdown signal (no connection /// task is listening). For tests and non-WS callers. pub fn register(&self, inst: Instance) -> u64 { @@ -603,6 +637,37 @@ mod tests { assert_eq!(r.expired(Duration::ZERO), vec![h]); } + #[test] + fn observer_cap_refuses_observers_but_never_agents() { + // Review round-10 F51 (facet d): the per-namespace observer ceiling + // is enforced atomically at registration — count and insert under one + // write-lock acquisition — and never applies to agents. + let r = Registry::new(); + let cap = 2; + for i in 0..cap { + let mut ob = inst("prod", &format!("lobby-{i}"), &format!("o-{i}"), 0); + ob.agent_type = AgentType::Observer; + assert!(r.register_conn_capped(ob, shutdown_signal(), cap).is_ok()); + } + // The next observer in the same namespace is refused with the count. + let mut over = inst("prod", "lobby-x", "o-x", 0); + over.agent_type = AgentType::Observer; + assert_eq!( + r.register_conn_capped(over, shutdown_signal(), cap), + Err(cap) + ); + // A different namespace has its own budget. + let mut other_ns = inst("dev", "lobby-0", "o-d", 0); + other_ns.agent_type = AgentType::Observer; + assert!(r + .register_conn_capped(other_ns, shutdown_signal(), cap) + .is_ok()); + // Agents are never subject to the observer cap. + assert!(r + .register_conn_capped(inst("prod", "w-extra", "i-w", 1), shutdown_signal(), cap) + .is_ok()); + } + #[test] fn observers_never_selectable_but_listed() { let r = Registry::new(); diff --git a/crates/openab-cp/src/router.rs b/crates/openab-cp/src/router.rs index b2fe8c284..be4485c02 100644 --- a/crates/openab-cp/src/router.rs +++ b/crates/openab-cp/src/router.rs @@ -661,11 +661,22 @@ impl Router { // Admission is committed: the token is minted, capacity is reserved, // and the entry is inserted — the duplicate check rides the in-flight // table, so nothing below needs the admission guard. Dropping it here - // keeps observer fan-out (serialization + per-observer try_send) out - // of the one critical section that serializes every delegation in - // every namespace: a saturated lobby must never slow admission. + // keeps the announce/forward critical section below out of the + // admission lock: that section holds the (also global) in-flight + // lock across the requested-emit and the forward — see the lock + // hierarchy note for why, and F51 in the review record for the + // acknowledged latency tradeoff. What dropping the guard buys is + // that new admissions in other connections are not serialized behind + // this delegation's fan-out. drop(admission); + // Everything expensive that does not need the table lock is computed + // BEFORE taking it: the prompt excerpt scans up to max_prompt_bytes + // of client input, and the forward frame was serialized above. The + // critical section below performs only the generation re-check, the + // announce flag, one bounded event emission, and non-blocking sends. + let prompt_excerpt = events.excerpt(from_namespace, &forward.prompt); + // Announce/forward critical section. One in-flight lock acquisition // covers three things that must be atomic with respect to entry // removal (see the module-level lock hierarchy note): @@ -687,12 +698,15 @@ impl Router { // forward — and run work every other party had already recorded // as cancelled, with its capacity reservation already released. // - // The work under the lock is bounded: one serialization, one - // non-blocking `try_send` per observer, one non-blocking `try_send` - // to the target. If a teardown or the deadline sweep removed the - // entry first, the admission is over before it was ever announced: - // emit nothing, forward nothing, report the loss to the initiator - // (whose own teardown is usually what removed it). + // Work under the lock: one event serialization (excerpt precomputed + // above, body bounded by the validated max_event_excerpt_bytes + // ceiling), one non-blocking `try_send` per observer (population + // bounded by max_observers_per_namespace at registration), and one + // non-blocking `try_send` to the target — bounded by configuration, + // not merely by expectation. If a teardown or the deadline sweep + // removed the entry first, the admission is over before it was ever + // announced: emit nothing, forward nothing, report the loss to the + // initiator (whose own teardown is usually what removed it). enum Forward { Sent, SendFailed(InFlight), @@ -711,7 +725,7 @@ impl Router { admission: entry.generation, from: entry.from_logical.clone(), to: entry.to_logical.clone(), - prompt_excerpt: events.excerpt(from_namespace, &forward.prompt), + prompt_excerpt, deadline: entry.deadline, chain: entry.chain.clone(), }, @@ -874,7 +888,8 @@ impl Router { } } - /// Phase 2 of a completion: end the delegation `peeked` describes. + /// The commit step of `complete`'s peek -> cap -> commit -> emit -> + /// deliver sequence: end the delegation `peeked` describes. /// /// Under ONE in-flight lock acquisition, the entry is removed and the /// serving instance's capacity released only if the live entry is still @@ -1040,7 +1055,7 @@ impl Router { // Phase 2 — cap the payload (in place: the capped value is what the // initiator and the lobby both see). - cap_result(&mut params, max_result_bytes); + cap_payload(&mut params, max_result_bytes); // Phase 3 — commit. Claims ONLY the admission that was peeked; see // `commit_completion` for why key + serving handle is not enough. @@ -1300,12 +1315,11 @@ impl Router { result: None, error: Some(error.clone()), }; - let frame = JsonRpcRequest::new( - rpc_id(), - methods::DELEGATE_RESULT, - Some(serde_json::to_value(¶ms).expect("serializable")), - ); - affected.push((init, serde_json::to_string(&frame).expect("serializable"))); + if let Some(text) = + synthesized_frame(rpc_id(), methods::DELEGATE_RESULT, ¶ms) + { + affected.push((init, text)); + } } // Emitted even when the initiator is already gone: the // lobby must see the delegation reach a terminal state. @@ -1339,13 +1353,9 @@ impl Router { admission: e.generation, reason: reason.clone(), }; - let frame = JsonRpcRequest::new( - rpc_id(), - methods::CANCEL, - Some(serde_json::to_value(¶ms).expect("serializable")), - ); - affected - .push((target, serde_json::to_string(&frame).expect("serializable"))); + if let Some(text) = synthesized_frame(rpc_id(), methods::CANCEL, ¶ms) { + affected.push((target, text)); + } } events.emit( registry, @@ -1420,12 +1430,9 @@ impl Router { result: None, error: Some("deadline exceeded".to_string()), }; - let frame = JsonRpcRequest::new( - rpc_id(), - methods::DELEGATE_RESULT, - Some(serde_json::to_value(¶ms).expect("serializable")), - ); - frames.push((init, serde_json::to_string(&frame).expect("serializable"))); + if let Some(text) = synthesized_frame(rpc_id(), methods::DELEGATE_RESULT, ¶ms) { + frames.push((init, text)); + } } if let Some(target) = registry.get(e.to_handle) { let params = CancelParams { @@ -1439,12 +1446,9 @@ impl Router { admission: e.generation, reason: "deadline exceeded".to_string(), }; - let frame = JsonRpcRequest::new( - rpc_id(), - methods::CANCEL, - Some(serde_json::to_value(¶ms).expect("serializable")), - ); - frames.push((target, serde_json::to_string(&frame).expect("serializable"))); + if let Some(text) = synthesized_frame(rpc_id(), methods::CANCEL, ¶ms) { + frames.push((target, text)); + } } } frames @@ -1491,7 +1495,7 @@ enum Deliver { /// same sender as `result`, so it gets the same bound — an uncapped error /// would ride up to the transport frame limit while the result beside it is /// capped. -fn cap_result(params: &mut DelegateResultParams, max_result_bytes: usize) { +fn cap_payload(params: &mut DelegateResultParams, max_result_bytes: usize) { if let Some(r) = ¶ms.result { if r.len() > max_result_bytes { params.result = Some(truncate_with_marker(r, max_result_bytes)); @@ -1537,6 +1541,29 @@ fn deliver_result( Deliver::Queued } +/// Serialize a CP-synthesized wire frame, failing SOFT. Callers include +/// `fail_instance` and `sweep_deadlines`, which are reachable from connection +/// teardown — `RegistrationGuard`'s Drop, possibly already unwinding, where a +/// second panic aborts the whole process — and from the lease sweeper. A +/// frame that cannot serialize (practically unreachable for these types) is +/// dropped with an error log instead of panicking: the peer reconciles via +/// its own deadline, and the CP stays up. +fn synthesized_frame(rpc_id: u64, method: &str, params: &impl serde::Serialize) -> Option { + match serde_json::to_value(params) + .and_then(|v| serde_json::to_string(&JsonRpcRequest::new(rpc_id, method, Some(v)))) + { + Ok(t) => Some(t), + Err(e) => { + tracing::error!( + method, + error = %e, + "CP-synthesized frame serialization failed — frame dropped" + ); + None + } + } +} + /// Truncate `s` to at most `cap` bytes, keeping the head and appending a /// marker. The marker counts against the cap: the returned value never /// exceeds `cap`, and cuts always land on UTF-8 char boundaries. Shared by @@ -2666,6 +2693,230 @@ type = "primary" ); } + #[test] + fn concurrent_completes_yield_exactly_one_claim() { + // Review round-10 F52 (completing R4-F34): two results for the same + // admission racing each other must resolve to exactly one authority. + // The commit is the serialization point, so driving both frames + // through the same peek->commit seams `complete` uses proves the + // exactly-once property at the racy boundary, and the full wire path + // confirms the loser is dropped end to end. + let mut w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.worker_rx.try_recv().unwrap(); + let admission = token(&w.router, "prod", "d-1"); + + // Both duplicate results pass the peek before either commits — the + // widest possible race window. + let peek_a = match w + .router + .peek_for_completion("prod", "d-1", w.h_worker, admission) + { + Peek::Serving(e) => e, + _ => panic!("expected the live admission"), + }; + let peek_b = match w + .router + .peek_for_completion("prod", "d-1", w.h_worker, admission) + { + Peek::Serving(e) => e, + _ => panic!("expected the live admission"), + }; + + // Exactly one commit claims; the other finds the entry gone. + assert_eq!( + w.router.commit_completion(&w.registry, &peek_a), + Commit::Claimed + ); + assert_eq!( + w.router.commit_completion(&w.registry, &peek_b), + Commit::Vanished + ); + assert_eq!( + w.registry.get(w.h_worker).unwrap().active_sessions, + 0, + "capacity released exactly once" + ); + + // A duplicate arriving through the full wire path after the first + // committed is dropped at the peek — no second delivery, no second + // terminal. + assert_eq!( + w.router.complete( + &w.registry, + &w.events, + w.h_worker, + result_of("d-1", admission, "dup"), + 1024, + 5 + ), + CompleteOutcome::Dropped + ); + assert!( + w.primary_rx.try_recv().is_err(), + "the seam-driven commit did not deliver, and the duplicate must not either" + ); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 1, "requested only — seam commits do not emit"); + assert_eq!(ev[0]["event"], "delegation_requested"); + } + + #[test] + fn teardown_of_unannounced_entry_target_side_is_silent_too() { + // Review round-10 F53: the initiator-teardown case is covered above; + // this is the symmetric case — the TARGET disconnects while the entry + // is inserted but not yet announced. Same contract: no synthesized + // result frame to the initiator (whose `cp/delegate` call is itself + // returning an error), no observer terminal, entry gone. + let w = world(); + let mut lobby = observe(&w, "prod"); + w.registry.adjust_sessions(w.h_worker, 1); + let entry = InFlight { + namespace: "prod".into(), + delegation_id: "d-race".into(), + from_logical: "prod/koudu".into(), + from_handle: w.h_primary, + to_logical: "prod/worker-1".into(), + to_handle: w.h_worker, + deadline: Utc::now() + Duration::seconds(60), + chain: vec!["prod/koudu".into()], + generation: 1, + announced: false, + }; + w.router + .inflight + .lock() + .insert(DelegationKey::new("prod", "d-race"), entry); + + let mut next = || 17; + let frames = w + .router + .fail_instance(&w.registry, &w.events, w.h_worker, &mut next); + + assert!( + frames.is_empty(), + "no synthesized result for a never-announced admission" + ); + assert!(events_of(&mut lobby).is_empty(), "no observer event"); + assert_eq!(w.router.inflight_count(), 0, "entry removed"); + } + + #[test] + fn dropped_and_stale_results_emit_no_observer_events() { + // Review round-10 F54 (completing R6-F17): the "dropped results emit + // nothing" invariant asserted with an observer attached, for both the + // stale-admission drop and the unknown-id drop. + let mut w = world(); + let mut lobby = observe(&w, "prod"); + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.worker_rx.try_recv().unwrap(); + let admission = token(&w.router, "prod", "d-1"); + // Drain the requested event. + assert_eq!(events_of(&mut lobby).len(), 1); + + // Stale admission token → dropped, no event, no delivery. + assert_eq!( + w.router.complete( + &w.registry, + &w.events, + w.h_worker, + result_of("d-1", admission + 999, "stale"), + 1024, + 6 + ), + CompleteOutcome::Dropped + ); + // Unknown id → dropped, no event, no delivery. + assert_eq!( + w.router.complete( + &w.registry, + &w.events, + w.h_worker, + result_of("d-unknown", 1, "ghost"), + 1024, + 7 + ), + CompleteOutcome::Dropped + ); + assert!(events_of(&mut lobby).is_empty(), "drops emit nothing"); + assert!( + w.primary_rx.try_recv().is_err(), + "drops deliver nothing to the initiator" + ); + assert_eq!(w.router.inflight_count(), 1, "live entry untouched"); + } + + #[test] + fn saturated_observer_crowd_does_not_block_delegation() { + // Review round-10 F51 (functional contention floor): a full cap's + // worth of observers, every one of them saturated, must not block or + // fail admission, forwarding, or completion — sends are non-blocking + // and the per-observer work is bounded. (Latency budgeting is the + // config cap's job; this pins the functional non-interference.) + let mut w = world(); + let mut lobbies = Vec::new(); + for i in 0..16 { + let (tx, rx) = crate::registry::outbound_channel(8); + w.registry.register(Instance { + handle: 0, + namespace: "prod".into(), + name: format!("lobby-{i}"), + agent_type: AgentType::Observer, + instance_id: format!("o-{i}"), + labels: Default::default(), + max_delegated_sessions: 0, + active_sessions: 0, + registered_at: Instant::now(), + last_heartbeat: Instant::now(), + tx, + }); + lobbies.push(rx); + } + // Saturate every observer queue (8-byte budgets refuse any frame + // after one filler). + for i in 0..16 { + let inst = w + .registry + .observers("prod") + .into_iter() + .find(|o| o.instance_id == format!("o-{i}")) + .unwrap(); + while inst.tx.try_send("12345678".into()).is_ok() {} + } + + assert!(matches!( + do_delegate(&w, delegate_params("d-1", "worker-1", 60)), + DelegateOutcome::Accepted(_) + )); + w.worker_rx.try_recv().expect("forward reached the worker"); + let admission = token(&w.router, "prod", "d-1"); + assert_eq!( + w.router.complete( + &w.registry, + &w.events, + w.h_worker, + result_of("d-1", admission, "done"), + 1024, + 8 + ), + CompleteOutcome::Completed { + delivered: true, + stalled_initiator: None + } + ); + assert!( + w.primary_rx.try_recv().unwrap().contains("delegate_result"), + "the initiator got its terminal despite the saturated lobby crowd" + ); + } + #[test] fn duplicate_delegation_id_rejected() { let w = world(); @@ -4513,6 +4764,15 @@ type = "primary" assert_eq!(ev[1]["event"], "delegation_completed"); assert_eq!(ev[1]["admission"], admission); + // The refused frame must not have reached the initiator: its queue + // holds only the filler frames this test packed it with. + while let Ok(f) = w.primary_rx.try_recv() { + assert!( + !f.contains("delegate_result"), + "a refused terminal frame must not be delivered" + ); + } + let mut next = || 9; w.router .fail_instance(&w.registry, &w.events, w.h_primary, &mut next); diff --git a/crates/openab-cp/src/server.rs b/crates/openab-cp/src/server.rs index 9418cc6e8..802d64ab7 100644 --- a/crates/openab-cp/src/server.rs +++ b/crates/openab-cp/src/server.rs @@ -326,7 +326,11 @@ async fn handle_connection( .effective_max_sessions(&identity, reg.max_delegated_sessions); // The registry assigns the CP-generated handle: ownership // and teardown never key on the client-supplied instance_id. - let handle = state.registry.register_conn( + // Observers are additionally bounded per namespace: fan-out does + // bounded per-observer work inside the delegation path's in-flight + // critical section, so the observer count is a configured latency + // budget, not an open-ended population. + let handle = match state.registry.register_conn_capped( Instance { handle: 0, namespace: identity.namespace.clone(), @@ -341,7 +345,38 @@ async fn handle_connection( tx: tx.clone(), }, Arc::clone(&shutdown), - ); + state.cfg.max_observers_per_namespace, + ) { + Ok(h) => h, + Err(current) => { + warn!( + agent = %format!("{}/{}", identity.namespace, identity.name), + observers = current, + max = state.cfg.max_observers_per_namespace, + "registration refused: namespace is at its observer cap" + ); + let resp = JsonRpcErrorResponse::new( + reg_rpc_id, + ErrorObject::new( + codes::SATURATED, + format!( + "namespace is at its observer cap \ + (max_observers_per_namespace = {}); retry later or \ + raise the cap", + state.cfg.max_observers_per_namespace + ), + ), + ); + let _ = send_bounded( + &mut sink, + Message::Text(serde_json::to_string(&resp).expect("serializable").into()), + write_timeout, + &mut shutdown_rx, + ) + .await; + return; + } + }; // From here on, teardown is owned by an RAII guard rather than the return // path: a panic anywhere below (the `expect("serializable")` sites are on // production paths) would otherwise skip deregistration and leave this @@ -520,10 +555,14 @@ struct RegistrationGuard { impl Drop for RegistrationGuard { fn drop(&mut self) { // Must not panic: a panic here during an unwind aborts the process. - // Everything it touches is lock-guarded map mutation and non-blocking - // sends — no `expect`, no allocation-dependent invariants. parking_lot - // locks are not poisoned and are released by the unwind itself, so a - // panic taken while holding one cannot deadlock this call. + // Teardown is lock-guarded map mutation, non-blocking sends, and + // FAIL-SOFT frame/event serialization: `fail_instance`, + // `sweep_deadlines`, and `EventHub::emit` drop a frame with an error + // log instead of panicking on a serialization error (see + // `synthesized_frame` and `emit`), so no `expect`/`unwrap` lies on + // this path. parking_lot locks are not poisoned and are released by + // the unwind itself, so a panic taken while holding one cannot + // deadlock this call. teardown(&self.state, self.handle, &self.identity); } } @@ -1215,8 +1254,17 @@ mod tests { let state = state_with(""); let (h_i, _rx_i) = register_test_instance(&state, "koudu", AgentType::Primary, 4); let (h_w, mut rx_w) = register_test_instance(&state, "worker-1", AgentType::Worker, 1); + // An observer is attached so the unwind exercises the FULL teardown + // emit surface — the deregister announcement and the per-admission + // terminal — which must be fail-soft: this Drop may already be + // unwinding, where a second panic aborts the process (review + // round-10 F62). + let (h_o, mut rx_o) = register_test_instance(&state, "lobby", AgentType::Observer, 0); + let _ = h_o; delegate_through_handler(&state, h_i, "d-1", "worker-1"); rx_w.try_recv().expect("worker received the forward"); + // Drain the events the delegation produced so far. + while rx_o.try_recv().is_ok() {} assert_eq!(state.registry.get(h_w).unwrap().active_sessions, 1); assert_eq!(state.router.inflight_count(), 1); @@ -1253,6 +1301,17 @@ mod tests { // ...and the serving runtime is told to stop working. let cancel = rx_w.try_recv().expect("downstream cancel was queued"); assert!(cancel.contains("cp/cancel") && cancel.contains("d-1")); + // The observer received the teardown's whole emit surface — produced + // during the unwind without a second panic: the deregister + // announcement and the delegation's terminal. + let mut saw_deregistered = false; + let mut saw_cancelled = false; + while let Ok(f) = rx_o.try_recv() { + saw_deregistered |= f.contains("agent_deregistered"); + saw_cancelled |= f.contains("delegation_cancelled"); + } + assert!(saw_deregistered, "deregister announcement emitted in Drop"); + assert!(saw_cancelled, "delegation terminal emitted in Drop"); } #[test] diff --git a/docs/adr/agent-control-plane.md b/docs/adr/agent-control-plane.md index f98c429e1..f5a5195d0 100644 --- a/docs/adr/agent-control-plane.md +++ b/docs/adr/agent-control-plane.md @@ -490,7 +490,9 @@ recovery semantics: initiator-bound terminal frame on the wire, and a result whose commit loses the race is discarded undelivered — so under one CP process an initiator receives at most one terminal frame per admission, and the - observer event stream records the same outcome. The rule is retained as + observer event stream records the committed outcome (an initiator whose + connection cannot accept its terminal frame is disconnected and misses it; + the stream still shows what the CP committed). The rule is retained as client-side defence in depth (frames straddling a CP restart, future multi-instance deployments): **initiators MUST treat the first terminal frame (`completed`, `failed`, `timeout`, `target_disconnected`) for a diff --git a/docs/control-plane.md b/docs/control-plane.md index fb9d2b6e6..dd4f358d9 100644 --- a/docs/control-plane.md +++ b/docs/control-plane.md @@ -168,9 +168,13 @@ Client contract: forward reaches the worker, and a terminal event is published only by the path that authoritatively ended the admission — so you never see a terminal before its `requested`, never more than one terminal per admission, and the - terminal you see is the same outcome the initiator was sent. An - `agent_deregistered` without a matching `agent_registered` is possible - (registration ack failure); treat removal of an unknown agent as a no-op. + terminal you see is the outcome the CP committed. One edge to know: if the + initiator's connection cannot accept its terminal frame (queue refused, or + it died first), the CP disconnects it and the frame is not delivered — the + observer still sees the committed outcome, which that initiator never + received. An `agent_deregistered` without a matching `agent_registered` is + possible (registration ack failure); treat removal of an unknown agent as + a no-op. - **Terminal asymmetry.** Completion-shaped endings (`completed`, `failed`, `timeout`, `target_disconnected`) arrive as `delegation_completed` with a `status`; cancellations (initiator cancel, initiator disconnect) arrive as @@ -178,7 +182,13 @@ Client contract: literal `"control-plane"` for CP-synthesized cancellations. - **Best-effort delivery.** Fan-out uses the same bounded per-connection queue as everything else: a lobby that cannot keep up loses frames and - detects it via the `seq` gap. Observers can never slow the delegation path. + detects it via the `seq` gap. Sends are non-blocking, so a single slow + observer cannot block a delegation — but fan-out serialization runs inside + the delegation path's in-flight critical section, so the observer + population adds bounded latency to delegation bookkeeping. The bound is + configuration, not hope: `max_observers_per_namespace` (default 16) caps + the population and `max_event_excerpt_bytes` (validated to at most 64 KiB) + caps the per-frame work. - **Content redaction.** Prompt/result excerpts are bounded by `max_event_excerpt_bytes` (default 4 KiB). In a `metadata_only = true` namespace, agent-supplied content (prompt/result excerpts, worker-reported From 6485afcdee974bfa9c83a743d5ae33c6b0ea1774 Mon Sep 17 00:00:00 2001 From: "chaodu-obk[bot]" <307341165+chaodu-obk[bot]@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:12:18 +0000 Subject: [PATCH 9/9] fix(review): aggregate drop logging, document SATURATED domains (round-12 findings) - F63: EventHub::emit now logs ONE aggregated warn per emission (drop count out of observer count) instead of one warn per refused observer, keeping systemic lobby saturation visible at default log levels without multiplying log I/O by observer count on the delegation path; per-observer detail moves to debug. - F64: recorded as an explicit decision instead of an accident - the SATURATED code doc in proto.rs now states the three capacity domains it deliberately covers and why one code suffices (same client reaction; every message names the exhausted bound), and the client doc's SATURATED bullet includes the observer-cap case. Revisit if machine-readable attribution is needed. - F65: dedicated regression pins the precomputed prompt-excerpt path: truncation preserved in a normal namespace, excerpt key absent under metadata_only, secret content nowhere in the frame. - F66: RegistrationGuard::drop doc rephrased - sweep_deadlines is teardown-adjacent (sweeper task), not on the Drop chain; the fail-soft discipline claim now cannot be misread. - Review NIT: register_conn_capped allocates the handle only after the cap check, so a refused registration no longer consumes a handle id. - Reviewer-suggested guard comment on deliver_result: its intentional expect (request path, absorbed by fail-soft teardown) is now documented so the asymmetry with synthesized_frame is not "fixed" blindly. cargo test -p openab-cp: 149 passed, 0 failed. clippy --all-targets -D warnings clean; rustfmt clean. --- crates/openab-cp/src/events.rs | 30 +++++++++++++---- crates/openab-cp/src/proto.rs | 15 +++++++-- crates/openab-cp/src/registry.rs | 6 ++-- crates/openab-cp/src/router.rs | 56 ++++++++++++++++++++++++++++++++ crates/openab-cp/src/server.rs | 14 ++++---- docs/control-plane.md | 6 ++-- 6 files changed, 109 insertions(+), 18 deletions(-) diff --git a/crates/openab-cp/src/events.rs b/crates/openab-cp/src/events.rs index 88a77cc01..0e37caa82 100644 --- a/crates/openab-cp/src/events.rs +++ b/crates/openab-cp/src/events.rs @@ -139,20 +139,38 @@ impl EventHub { return; } }; - for o in observers { + let mut dropped = 0usize; + for o in &observers { // Best effort by design: a saturated lobby queue drops the frame. - // Logged at warn: systemic frame loss (a saturated lobby) must be - // visible at default log levels, not only under debug. if o.tx.try_send(text.clone()).is_err() { - tracing::warn!( + dropped += 1; + // Per-observer detail stays at debug; the aggregated warn + // below is the default-level signal. + tracing::debug!( observer = %o.logical_id(), instance = %o.instance_id, seq = params.seq, - "observer queue full or closed — event frame dropped \ - (client resyncs via the seq gap)" + "observer queue full or closed — event frame dropped" ); } } + // One aggregated warn per emission, not one per observer: systemic + // lobby saturation must be visible at default log levels, but a + // persistently saturated full-cap namespace must not multiply log + // I/O by observer count on the delegation path (review round-12 + // F63 — this emit can run inside the router's in-flight critical + // section). + if dropped > 0 { + tracing::warn!( + namespace, + seq = params.seq, + dropped, + observers = observers.len(), + "observer queues full or closed — event frame dropped for \ + {dropped} of {} observers (clients resync via the seq gap)", + observers.len() + ); + } } } diff --git a/crates/openab-cp/src/proto.rs b/crates/openab-cp/src/proto.rs index c7f3ad163..5e70b984b 100644 --- a/crates/openab-cp/src/proto.rs +++ b/crates/openab-cp/src/proto.rs @@ -157,8 +157,19 @@ pub mod codes { pub const POLICY_DENIED: i64 = -32003; /// No registered, healthy runtime matches the target selector. pub const NO_TARGET: i64 = -32004; - /// Matching targets exist but all are at their advertised capacity. - /// Explicit fast-fail: the CP never queues (v1 has no durable state). + /// A capacity ceiling is exhausted. Explicit fast-fail: the CP never + /// queues (v1 has no durable state). + /// + /// Deliberately ONE code for three capacity domains — all matching + /// targets at their advertised capacity, the CP at its global + /// `max_inflight_delegations` ceiling, or a namespace at its + /// `max_observers_per_namespace` ceiling (registration refusal). Every + /// message names the exhausted bound and, where applicable, the config + /// knob, so a human can attribute the refusal; a client's reaction is + /// the same in all three cases (back off / retry / raise the bound), so + /// splitting the code would grow the wire surface without changing any + /// client decision. Revisit if machine-readable attribution is ever + /// needed for alerting. pub const SATURATED: i64 = -32005; // -32006 is deliberately unassigned. It held a `DEADLINE_EXCEEDED` code // that nothing could ever emit: a deadline already in the past is diff --git a/crates/openab-cp/src/registry.rs b/crates/openab-cp/src/registry.rs index db515f068..f81c34981 100644 --- a/crates/openab-cp/src/registry.rs +++ b/crates/openab-cp/src/registry.rs @@ -285,8 +285,6 @@ impl Registry { shutdown: ShutdownTx, max_observers_per_namespace: usize, ) -> Result { - let handle = self.next_handle.fetch_add(1, Ordering::Relaxed) + 1; - inst.handle = handle; let mut g = self.inner.write(); if inst.agent_type == AgentType::Observer { let observers = g @@ -299,6 +297,10 @@ impl Registry { return Err(observers); } } + // Allocated only after the cap check: a refused registration must + // not consume a handle id. + let handle = self.next_handle.fetch_add(1, Ordering::Relaxed) + 1; + inst.handle = handle; g.insert(handle, Entry { inst, shutdown }); Ok(handle) } diff --git a/crates/openab-cp/src/router.rs b/crates/openab-cp/src/router.rs index be4485c02..cc4089834 100644 --- a/crates/openab-cp/src/router.rs +++ b/crates/openab-cp/src/router.rs @@ -1509,6 +1509,15 @@ fn cap_payload(params: &mut DelegateResultParams, max_result_bytes: usize) { } /// Queue the terminal result on the initiator's connection. +/// +/// The `expect("serializable")` here is INTENTIONAL, unlike the fail-soft +/// [`synthesized_frame`] used by teardown paths: this runs on the +/// `cp/delegate_result` request path (never from a Drop or the sweeper), its +/// params were just deserialized from a client frame (provably +/// serializable), and a panic here is absorbed by `RegistrationGuard`'s +/// fail-soft teardown. Do not convert it to fail-soft without understanding +/// that distinction — silently losing a genuine terminal on the request path +/// is worse than the loud failure. fn deliver_result( registry: &Registry, entry: &InFlight, @@ -2853,6 +2862,53 @@ type = "primary" assert_eq!(w.router.inflight_count(), 1, "live entry untouched"); } + #[test] + fn precomputed_prompt_excerpt_is_truncated_and_metadata_only_aware() { + // Review round-12 F65: the prompt excerpt is computed BEFORE the + // announce/forward critical section and passed in as a value. This + // pins the precomputed path directly: the requested event carries a + // properly truncated excerpt in a normal namespace, and no excerpt + // key at all in a metadata_only namespace — proving the hoisting + // changed neither the truncation nor the redaction behavior. + let w = world(); + let mut lobby = observe(&w, "prod"); + let mut p = delegate_params("d-big", "worker-1", 60); + p.prompt = "p".repeat(64 * 1024); + assert!(matches!(do_delegate(&w, p), DelegateOutcome::Accepted(_))); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 1); + assert_eq!(ev[0]["event"], "delegation_requested"); + let excerpt = ev[0]["prompt_excerpt"].as_str().unwrap(); + assert!(excerpt.contains("truncated by control plane")); + assert!( + excerpt.len() <= 4096, + "excerpt bounded by max_event_excerpt_bytes: {}", + excerpt.len() + ); + + // metadata_only namespace: the key is absent entirely. + let cfg: CpConfig = toml::from_str( + r#" +[namespaces.prod] +metadata_only = true +"#, + ) + .unwrap(); + let w = world_with_cfg(cfg); + let mut lobby = observe(&w, "prod"); + let mut p = delegate_params("d-quiet", "worker-1", 60); + p.prompt = "secret payload".into(); + assert!(matches!(do_delegate(&w, p), DelegateOutcome::Accepted(_))); + let ev = events_of(&mut lobby); + assert_eq!(ev.len(), 1); + assert_eq!(ev[0]["event"], "delegation_requested"); + assert!( + ev[0].get("prompt_excerpt").is_none(), + "metadata_only suppresses the excerpt key entirely" + ); + assert!(!ev[0].to_string().contains("secret payload")); + } + #[test] fn saturated_observer_crowd_does_not_block_delegation() { // Review round-10 F51 (functional contention floor): a full cap's diff --git a/crates/openab-cp/src/server.rs b/crates/openab-cp/src/server.rs index 802d64ab7..c442d6d39 100644 --- a/crates/openab-cp/src/server.rs +++ b/crates/openab-cp/src/server.rs @@ -556,12 +556,14 @@ impl Drop for RegistrationGuard { fn drop(&mut self) { // Must not panic: a panic here during an unwind aborts the process. // Teardown is lock-guarded map mutation, non-blocking sends, and - // FAIL-SOFT frame/event serialization: `fail_instance`, - // `sweep_deadlines`, and `EventHub::emit` drop a frame with an error - // log instead of panicking on a serialization error (see - // `synthesized_frame` and `emit`), so no `expect`/`unwrap` lies on - // this path. parking_lot locks are not poisoned and are released by - // the unwind itself, so a panic taken while holding one cannot + // FAIL-SOFT frame/event serialization. This Drop chain reaches + // `fail_instance` and `EventHub::emit`; the teardown-adjacent + // `sweep_deadlines` (called from the lease sweeper task, not from + // here) shares the same fail-soft discipline. All three drop a frame + // with an error log instead of panicking on a serialization error + // (see `synthesized_frame` and `emit`), so no `expect`/`unwrap` lies + // on this path. parking_lot locks are not poisoned and are released + // by the unwind itself, so a panic taken while holding one cannot // deadlock this call. teardown(&self.state, self.handle, &self.identity); } diff --git a/docs/control-plane.md b/docs/control-plane.md index dd4f358d9..e29564dc9 100644 --- a/docs/control-plane.md +++ b/docs/control-plane.md @@ -113,8 +113,10 @@ issue #1474). would otherwise mask the retry's genuine result. Every terminal frame carries the token, including CP-synthesized `timeout` and `target_disconnected`. - A delegation may be refused with `SATURATED` because the target is at - capacity *or* because the CP is at `max_inflight_delegations`; the error - message says which. The CP never queues — retry later. + capacity *or* because the CP is at `max_inflight_delegations`; an observer + registration may be refused with the same code when its namespace is at + `max_observers_per_namespace`. The error message always says which bound + was hit. The CP never queues — retry later (or raise the named knob). - The capacity a runtime advertises in `max_delegated_sessions` is clamped by the CP (`default_max_delegated_sessions_cap`, or a per-identity override). The ack's `effective_max_delegated_sessions` is the value that counts.