fix(core): cron double-fire, materialize warnings, group quota fast path, SSE channel cap
Remediate the MEDIUM backend correctness/perf findings from the 2026-07-11 audit. B1 — a group cron template toggled disabled→enabled no longer re-fires every descendant's cron. Materialization now KEEPS a disabled cron template's copies (syncing `enabled` in place) instead of delete+recreate, preserving `last_fired_at`; the scheduler already skips disabled copies, so keeping them is inert. Queue/email arms still delete-on-disable (freeing the one-consumer slot). B2 — `rematerialize_stateful_templates` warnings are now logged at the app-create/delete and group-reparent chokepoints (were silently dropped); only the `Err` arm was handled before. B3 — the group KV/docs byte-quota check skips the O(rows) `SUM(octet_length(..))` scan when a cheap upper bound (`rows_after * max_value_bytes`) is already under the cap, so the full aggregate only runs near-cap. Every row is validated ≤ max_value_bytes, so the bound is sound (never lets an over-quota write through). B6 — the in-process SSE broadcaster caps live channels per map (`PICLOUD_REALTIME_MAX_CHANNELS`, default 100k); a subscribe that would open a NEW channel past the cap is refused with 503 + `Retry-After: 1` rather than growing the (app,topic)/(group,topic) maps unboundedly. Check+insert is atomic under the map mutex (no TOCTOU); an existing-channel subscribe is exempt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -32,6 +32,12 @@ use tokio::sync::broadcast;
|
||||
pub const DEFAULT_BROADCAST_CAPACITY: usize = 64;
|
||||
const ENV_CAPACITY: &str = "PICLOUD_REALTIME_BROADCAST_CAPACITY";
|
||||
|
||||
/// Default cap on the number of live channels PER map (per-app and per-group).
|
||||
/// A subscribe that would create a NEW channel past this is refused, so an
|
||||
/// attacker naming unbounded distinct topics can't grow the maps until OOM.
|
||||
pub const DEFAULT_MAX_CHANNELS: usize = 100_000;
|
||||
const ENV_MAX_CHANNELS: &str = "PICLOUD_REALTIME_MAX_CHANNELS";
|
||||
|
||||
/// Default GC sweep interval for empty channels.
|
||||
pub const DEFAULT_GC_INTERVAL_SECS: u64 = 60;
|
||||
|
||||
@@ -41,6 +47,8 @@ pub struct InProcessBroadcaster {
|
||||
/// subtree app subscribing to the same shared topic shares one channel.
|
||||
groups: Mutex<HashMap<(GroupId, String), broadcast::Sender<RealtimeEvent>>>,
|
||||
capacity: usize,
|
||||
/// Max live channels per map (see [`DEFAULT_MAX_CHANNELS`]).
|
||||
max_channels: usize,
|
||||
}
|
||||
|
||||
impl InProcessBroadcaster {
|
||||
@@ -50,10 +58,19 @@ impl InProcessBroadcaster {
|
||||
inner: Mutex::new(HashMap::new()),
|
||||
groups: Mutex::new(HashMap::new()),
|
||||
capacity: capacity.max(1),
|
||||
max_channels: DEFAULT_MAX_CHANNELS,
|
||||
}
|
||||
}
|
||||
|
||||
/// Build from `PICLOUD_REALTIME_BROADCAST_CAPACITY` (default 64).
|
||||
/// Override the per-map channel cap (0 → treated as 1; never unbounded).
|
||||
#[must_use]
|
||||
pub fn with_max_channels(mut self, max_channels: usize) -> Self {
|
||||
self.max_channels = max_channels.max(1);
|
||||
self
|
||||
}
|
||||
|
||||
/// Build from `PICLOUD_REALTIME_BROADCAST_CAPACITY` (default 64) and
|
||||
/// `PICLOUD_REALTIME_MAX_CHANNELS` (default 100_000).
|
||||
#[must_use]
|
||||
pub fn from_env() -> Self {
|
||||
let capacity = match std::env::var(ENV_CAPACITY) {
|
||||
@@ -70,7 +87,21 @@ impl InProcessBroadcaster {
|
||||
}
|
||||
},
|
||||
};
|
||||
Self::new(capacity)
|
||||
let max_channels = match std::env::var(ENV_MAX_CHANNELS) {
|
||||
Err(_) => DEFAULT_MAX_CHANNELS,
|
||||
Ok(v) => match v.parse::<usize>() {
|
||||
Ok(n) if n > 0 => n,
|
||||
Ok(_) => {
|
||||
tracing::warn!(env = ENV_MAX_CHANNELS, value = %v, "must be > 0; using default");
|
||||
DEFAULT_MAX_CHANNELS
|
||||
}
|
||||
Err(e) => {
|
||||
tracing::warn!(env = ENV_MAX_CHANNELS, value = %v, error = %e, "invalid; using default");
|
||||
DEFAULT_MAX_CHANNELS
|
||||
}
|
||||
},
|
||||
};
|
||||
Self::new(capacity).with_max_channels(max_channels)
|
||||
}
|
||||
|
||||
/// Number of live channels in both maps (test/observability helper).
|
||||
@@ -110,8 +141,16 @@ impl RealtimeBroadcaster for InProcessBroadcaster {
|
||||
.inner
|
||||
.lock()
|
||||
.map_err(|_| BroadcasterError::Unavailable("broadcaster map poisoned".into()))?;
|
||||
let key = (app_id, topic.to_string());
|
||||
// Refuse to open a NEW channel past the cap; subscribing to an existing
|
||||
// one (another receiver) is always allowed.
|
||||
if !g.contains_key(&key) && g.len() >= self.max_channels {
|
||||
return Err(BroadcasterError::Unavailable(
|
||||
"realtime channel limit reached".into(),
|
||||
));
|
||||
}
|
||||
let tx = g
|
||||
.entry((app_id, topic.to_string()))
|
||||
.entry(key)
|
||||
.or_insert_with(|| broadcast::channel(self.capacity).0);
|
||||
Ok(tx.subscribe())
|
||||
}
|
||||
@@ -145,8 +184,14 @@ impl RealtimeBroadcaster for InProcessBroadcaster {
|
||||
.groups
|
||||
.lock()
|
||||
.map_err(|_| BroadcasterError::Unavailable("group broadcaster map poisoned".into()))?;
|
||||
let key = (group_id, topic.to_string());
|
||||
if !g.contains_key(&key) && g.len() >= self.max_channels {
|
||||
return Err(BroadcasterError::Unavailable(
|
||||
"realtime channel limit reached".into(),
|
||||
));
|
||||
}
|
||||
let tx = g
|
||||
.entry((group_id, topic.to_string()))
|
||||
.entry(key)
|
||||
.or_insert_with(|| broadcast::channel(self.capacity).0);
|
||||
Ok(tx.subscribe())
|
||||
}
|
||||
@@ -308,6 +353,23 @@ mod tests {
|
||||
assert_eq!(b.channel_count(), 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn subscribe_refuses_new_channel_past_the_cap() {
|
||||
// Bound the maps: a subscribe that would open a NEW channel past the cap
|
||||
// is refused, but subscribing to an already-open channel still works.
|
||||
let b = InProcessBroadcaster::new(16).with_max_channels(2);
|
||||
let app = AppId::new();
|
||||
let _rx1 = b.subscribe(app, "a").await.unwrap();
|
||||
let _rx2 = b.subscribe(app, "b").await.unwrap();
|
||||
// Third distinct topic → over cap → refused.
|
||||
assert!(matches!(
|
||||
b.subscribe(app, "c").await,
|
||||
Err(BroadcasterError::Unavailable(_))
|
||||
));
|
||||
// An additional receiver on an existing channel is still allowed.
|
||||
assert!(b.subscribe(app, "a").await.is_ok());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn drop_group_topic_disconnects_subscribers() {
|
||||
let b = InProcessBroadcaster::new(16);
|
||||
|
||||
@@ -134,14 +134,7 @@ async fn sse_topic(
|
||||
// 4. Subscribe + stream.
|
||||
let rx = match state.broadcaster.subscribe(app_id, &topic).await {
|
||||
Ok(rx) => rx,
|
||||
Err(e) => {
|
||||
tracing::error!(error = %e, "failed to acquire realtime subscription");
|
||||
return (
|
||||
StatusCode::INTERNAL_SERVER_ERROR,
|
||||
axum::Json(serde_json::json!({ "error": "internal error" })),
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
Err(e) => return subscribe_unavailable(&e),
|
||||
};
|
||||
|
||||
let stream = event_stream(rx);
|
||||
@@ -199,14 +192,7 @@ async fn sse_shared_topic(
|
||||
|
||||
let rx = match state.broadcaster.subscribe_group(group_id, &topic).await {
|
||||
Ok(rx) => rx,
|
||||
Err(e) => {
|
||||
tracing::error!(error = %e, "failed to acquire shared-topic subscription");
|
||||
return (
|
||||
StatusCode::INTERNAL_SERVER_ERROR,
|
||||
axum::Json(serde_json::json!({ "error": "internal error" })),
|
||||
)
|
||||
.into_response();
|
||||
}
|
||||
Err(e) => return subscribe_unavailable(&e),
|
||||
};
|
||||
|
||||
let sse = Sse::new(event_stream(rx))
|
||||
@@ -264,6 +250,20 @@ fn unauthorized() -> Response {
|
||||
.into_response()
|
||||
}
|
||||
|
||||
/// A `subscribe`/`subscribe_group` failure. `BroadcasterError` is transient by
|
||||
/// construction (the channel cap is reached, or a rare poisoned lock / cluster
|
||||
/// register failure), so it maps to 503 + `Retry-After: 1` — a "come back
|
||||
/// later", not a 500. Aligns with the `PICLOUD_REALTIME_MAX_CHANNELS` doc.
|
||||
fn subscribe_unavailable(e: &picloud_shared::BroadcasterError) -> Response {
|
||||
tracing::warn!(error = %e, "realtime subscription refused (broadcaster unavailable)");
|
||||
(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
[(axum::http::header::RETRY_AFTER, "1")],
|
||||
axum::Json(serde_json::json!({ "error": "realtime broadcaster unavailable" })),
|
||||
)
|
||||
.into_response()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
Reference in New Issue
Block a user