From a867bbd29f7d7b7ecd92ff02e1a439ea5a0d9e56 Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Fri, 31 Jul 2026 12:05:46 +0200 Subject: [PATCH 1/9] feat(error-tracking): resolve event-level release in cymbal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Master removed the frame-derived $exception_releases map in #75456, so cymbal currently emits no exception release at all. This restores it at the event level, resolved independently of which symbol set resolved the stack: - $release_id (web builds inject it) -> direct foreign-key lookup. - app metadata (mobile) -> reconstruct the release hash from $app_namespace and pack_version($app_version, $app_build), then look it up. Adds ReleaseRecord::for_id / for_hash and a per-worker ReleaseCache (moka) so the per-event lookups don't re-hit Postgres, and surfaces the result as the singular $exception_release. The cache loads through try_get_with, so concurrent misses on the same key coalesce into one query instead of one per event, and negative results are cached too — that is what removes the per-event query for apps that never bound a release. The cache is bounded in bytes rather than entries, since a release carries a free-form metadata JSON column that any client can write, so entry count says nothing about the memory held. Negative entries are charged a fixed floor so they can't accumulate for free — misses are the high-cardinality side, one per app that never bound a release. Co-Authored-By: Claude Opus 5 (1M context) --- ...99b3871d8bd81f554481cc59a6d4cfc3c1ef4.json | 59 ++++ ...ee2bc51ef8874660ae712bf7d419bc971a76f.json | 59 ++++ rust/cymbal/src/core/metric_consts.rs | 2 + rust/cymbal/src/core/types/frames/mod.rs | 2 + rust/cymbal/src/core/types/frames/releases.rs | 223 +++++++++++++++ .../src/modes/processing/app_context.rs | 11 + rust/cymbal/src/modes/processing/config.rs | 11 + .../processing/stages/rate_limiting/mod.rs | 1 + .../stages/resolution/event_release.rs | 253 ++++++++++++++++++ .../processing/stages/resolution/frame.rs | 3 + .../modes/processing/stages/resolution/mod.rs | 16 ++ .../modes/processing/types/exception_event.rs | 98 ++++++- rust/cymbal/src/modes/processing/types/mod.rs | 8 + rust/cymbal/src/modes/resolution/service.rs | 5 + rust/cymbal/tests/common/mod.rs | 5 + rust/cymbal/tests/remote_resolution.rs | 3 + rust/cymbal/tests/remote_resolution_parity.rs | 5 + 17 files changed, 762 insertions(+), 2 deletions(-) create mode 100644 rust/cymbal/.sqlx/query-14e4503d0bcc1f5191f43c94ef499b3871d8bd81f554481cc59a6d4cfc3c1ef4.json create mode 100644 rust/cymbal/.sqlx/query-15ae3c40f2b589c586bb1da303fee2bc51ef8874660ae712bf7d419bc971a76f.json create mode 100644 rust/cymbal/src/core/types/frames/releases.rs create mode 100644 rust/cymbal/src/modes/processing/stages/resolution/event_release.rs diff --git a/rust/cymbal/.sqlx/query-14e4503d0bcc1f5191f43c94ef499b3871d8bd81f554481cc59a6d4cfc3c1ef4.json b/rust/cymbal/.sqlx/query-14e4503d0bcc1f5191f43c94ef499b3871d8bd81f554481cc59a6d4cfc3c1ef4.json new file mode 100644 index 000000000000..51460a2aea8f --- /dev/null +++ b/rust/cymbal/.sqlx/query-14e4503d0bcc1f5191f43c94ef499b3871d8bd81f554481cc59a6d4cfc3c1ef4.json @@ -0,0 +1,59 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT id, team_id, hash_id, created_at, version, project, metadata\n FROM posthog_errortrackingrelease\n WHERE id = $1 AND team_id = $2\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Uuid" + }, + { + "ordinal": 1, + "name": "team_id", + "type_info": "Int4" + }, + { + "ordinal": 2, + "name": "hash_id", + "type_info": "Text" + }, + { + "ordinal": 3, + "name": "created_at", + "type_info": "Timestamptz" + }, + { + "ordinal": 4, + "name": "version", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "project", + "type_info": "Text" + }, + { + "ordinal": 6, + "name": "metadata", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Uuid", + "Int4" + ] + }, + "nullable": [ + false, + false, + false, + false, + false, + false, + true + ] + }, + "hash": "14e4503d0bcc1f5191f43c94ef499b3871d8bd81f554481cc59a6d4cfc3c1ef4" +} diff --git a/rust/cymbal/.sqlx/query-15ae3c40f2b589c586bb1da303fee2bc51ef8874660ae712bf7d419bc971a76f.json b/rust/cymbal/.sqlx/query-15ae3c40f2b589c586bb1da303fee2bc51ef8874660ae712bf7d419bc971a76f.json new file mode 100644 index 000000000000..5155488f89a4 --- /dev/null +++ b/rust/cymbal/.sqlx/query-15ae3c40f2b589c586bb1da303fee2bc51ef8874660ae712bf7d419bc971a76f.json @@ -0,0 +1,59 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT id, team_id, hash_id, created_at, version, project, metadata\n FROM posthog_errortrackingrelease\n WHERE hash_id = $1 AND team_id = $2\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Uuid" + }, + { + "ordinal": 1, + "name": "team_id", + "type_info": "Int4" + }, + { + "ordinal": 2, + "name": "hash_id", + "type_info": "Text" + }, + { + "ordinal": 3, + "name": "created_at", + "type_info": "Timestamptz" + }, + { + "ordinal": 4, + "name": "version", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "project", + "type_info": "Text" + }, + { + "ordinal": 6, + "name": "metadata", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text", + "Int4" + ] + }, + "nullable": [ + false, + false, + false, + false, + false, + false, + true + ] + }, + "hash": "15ae3c40f2b589c586bb1da303fee2bc51ef8874660ae712bf7d419bc971a76f" +} diff --git a/rust/cymbal/src/core/metric_consts.rs b/rust/cymbal/src/core/metric_consts.rs index f9972ccce3b4..2568a40cb759 100644 --- a/rust/cymbal/src/core/metric_consts.rs +++ b/rust/cymbal/src/core/metric_consts.rs @@ -130,6 +130,8 @@ pub const SPIKE_ALERT_STAGE: &str = "cymbal_spike_detection_time"; pub const FRAME_RESOLVER_OPERATOR: &str = "cymbal_frame_batch_time"; pub const EXCEPTION_RESOLVER_OPERATOR: &str = "cymbal_exception_exception_resolver_operator"; pub const LEGACY_ORDER_RESOLVER_OPERATOR: &str = "cymbal_exception_legacy_order_resolver_operator"; +pub const EVENT_RELEASE_RESOLVER_OPERATOR: &str = + "cymbal_exception_event_release_resolver_operator"; pub const LEGACY_ORDER_RESOLVE_FAILED: &str = "cymbal_exception_legacy_order_resolve_failed"; pub const FINGERPRINT_LEGACY_VERSION_USED: &str = "cymbal_fingerprint_legacy_version_used"; pub const ISSUE_LINKER_OPERATOR: &str = "cymbal_exception_issue_linker_operator"; diff --git a/rust/cymbal/src/core/types/frames/mod.rs b/rust/cymbal/src/core/types/frames/mod.rs index ae87fdde8055..5b06b43a60f4 100644 --- a/rust/cymbal/src/core/types/frames/mod.rs +++ b/rust/cymbal/src/core/types/frames/mod.rs @@ -39,6 +39,8 @@ pub(crate) fn record_frame_resolution_failure( tracing::debug!(lang = lang, reason = reason, error = %err, "frame resolution failed"); } +pub mod releases; + // We consume a huge variety of differently shaped stack frames, which we have special-case // transformation for, to produce a single, unified representation of a frame. #[derive(Debug, Deserialize, Serialize, Clone)] diff --git a/rust/cymbal/src/core/types/frames/releases.rs b/rust/cymbal/src/core/types/frames/releases.rs new file mode 100644 index 000000000000..e64c6c0edea8 --- /dev/null +++ b/rust/cymbal/src/core/types/frames/releases.rs @@ -0,0 +1,223 @@ +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; +use serde_json::Value; +use sha2::{Digest, Sha512}; +use sqlx::Executor; +use uuid::Uuid; + +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct ReleaseRecord { + pub id: Uuid, + pub team_id: i32, + pub hash_id: String, + pub created_at: DateTime, + pub version: String, + pub project: String, + pub metadata: Option, +} + +// The info, as written to clickhouse at the exception level. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReleaseInfo { + version: String, + project: String, + timestamp: DateTime, + #[serde(skip_serializing_if = "Option::is_none")] + metadata: Option, +} + +impl ReleaseRecord { + pub async fn for_id<'c, E>(e: E, id: Uuid, team_id: i32) -> Result, sqlx::Error> + where + E: Executor<'c, Database = sqlx::Postgres>, + { + let row = sqlx::query_as!( + Self, + r#" + SELECT id, team_id, hash_id, created_at, version, project, metadata + FROM posthog_errortrackingrelease + WHERE id = $1 AND team_id = $2 + "#, + id, + team_id + ) + .fetch_optional(e) + .await?; + + Ok(row) + } + + pub async fn for_hash<'c, E>( + e: E, + hash_id: &str, + team_id: i32, + ) -> Result, sqlx::Error> + where + E: Executor<'c, Database = sqlx::Postgres>, + { + let row = sqlx::query_as!( + Self, + r#" + SELECT id, team_id, hash_id, created_at, version, project, metadata + FROM posthog_errortrackingrelease + WHERE hash_id = $1 AND team_id = $2 + "#, + hash_id, + team_id + ) + .fetch_optional(e) + .await?; + + Ok(row) + } + + pub fn to_info(&self) -> ReleaseInfo { + ReleaseInfo { + project: self.project.clone(), + version: self.version.clone(), + timestamp: self.created_at, + metadata: self.metadata.clone(), + } + } + + /// Rough in-memory footprint, for the release cache's weigher. `metadata` is a free-form + /// JSON column any client can write, so it dominates and is the only reason this exists — + /// without it a cache bounded on entry count would be unbounded in bytes. Only has to be + /// proportional to the real cost, not exact. + pub fn approx_size_bytes(&self) -> usize { + size_of::() + + self.hash_id.len() + + self.version.len() + + self.project.len() + + self.metadata.as_ref().map_or(0, json_size_bytes) + } +} + +/// Heap bytes held by a `Value`, ignoring the inline scalars already counted by `size_of`. +/// +/// The recursion is bounded: these values are decoded by `serde_json`, which enforces its own +/// nesting limit while parsing, so a hostile `metadata` column can't drive this deep enough to +/// overflow the stack. +fn json_size_bytes(value: &Value) -> usize { + match value { + Value::Null | Value::Bool(_) | Value::Number(_) => 0, + Value::String(s) => s.len(), + Value::Array(items) => { + items.len() * size_of::() + items.iter().map(json_size_bytes).sum::() + } + Value::Object(entries) => entries + .iter() + .map(|(key, val)| key.len() + size_of::() + json_size_bytes(val)) + .sum(), + } +} + +/// Reconstruct the release `hash_id` the CLI wrote for a mobile build, from the app metadata the +/// SDK sends on every event. Mobile events carry no injected `$release_id`, so this is how their +/// release is resolved. It must stay byte-for-byte identical to the CLI, which keys releases on +/// `content_hash([name, version])` where `name` is the bundle identifier and `version` is +/// `pack_version(short_version, build)`: +/// - packing lives in `cli/src/sourcemaps/args.rs::pack_version` +/// - hashing lives in `cli/src/utils/files/content.rs::content_hash` (SHA-512 over the name bytes +/// followed by the version bytes, with no separator) +pub fn mobile_release_hash_id( + namespace: &str, + version: Option<&str>, + build: Option<&str>, +) -> Option { + let packed = pack_version(version, build)?; + Some(release_hash_id(namespace, &packed)) +} + +fn pack_version(version: Option<&str>, build: Option<&str>) -> Option { + match (version, build) { + (Some(v), Some(b)) => Some(format!("{v}+{b}")), + (Some(v), None) => Some(v.to_string()), + (None, Some(b)) => Some(b.to_string()), + (None, None) => None, + } +} + +fn release_hash_id(name: &str, version: &str) -> String { + let mut hasher = Sha512::new(); + hasher.update(name.as_bytes()); + hasher.update(version.as_bytes()); + format!("{:x}", hasher.finalize()) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + /// Golden vectors pinning the release `hash_id`: `content_hash([name, pack_version(version, + /// build)])`. Identical to the CLI's `release_hash_id_golden_vectors` test in + /// `cli/src/sourcemaps/args.rs`. The CLI computes this hash when it creates a release and cymbal + /// reconstructs it from a mobile event's app metadata, so these literals must stay the same on + /// both sides or mobile releases silently stop resolving. Keep the two tests in sync. + #[test] + fn release_hash_id_golden_vectors() { + // (name, version, build, expected hash_id) + let cases: [(&str, Option<&str>, Option<&str>, &str); 4] = [ + ( + "com.posthog.iosraw", + Some("1.0"), + Some("1"), + "75605cac5268ba4bdc57b4c8336f6686802e88236ae4026418a18cabcde854d1015f18734489b8ec4c71c68773a027e5b880f7278b8ba6864a5334d76ef9eba6", + ), + ( + "com.example.app", + Some("1.0"), + Some("42"), + "5a7f7b504d81759fa4e15f8b3bbc77c694a9dc222cfcd06c801fae9619076e97909edf651087106af331aea76463449f015ccc41ccacbf19148329b1c2c35aa7", + ), + ( + "com.example.app", + Some("2.3"), + None, + "09aeeb69b914985562d4aa39d13033abf0f90c753ef90b0148cb06b8aeadca7dd1dd853fa24c7cc51d18cf251bb7348eae58906347a217a98d74ba7ca5673b66", + ), + ( + "com.example.app", + None, + Some("99"), + "5e925a3f2e9349f64ab88eede466b641a7332dc79d6f1901d931fb659704a0475fa77a3ca25c0a60b2919547de8d94117fbcc52448e83aa72787a3fe35f725ae", + ), + ]; + + for (name, version, build, expected) in cases { + assert_eq!( + mobile_release_hash_id(name, version, build).as_deref(), + Some(expected), + "release hash_id drift for {name} {version:?}+{build:?}" + ); + } + } + + fn record(metadata: Option) -> ReleaseRecord { + ReleaseRecord { + id: Uuid::nil(), + team_id: 1, + hash_id: "hash".to_string(), + created_at: Utc::now(), + version: "1.0".to_string(), + project: "com.app".to_string(), + metadata, + } + } + + #[test] + fn size_estimate_tracks_metadata_payload() { + // The cache weigher is only a real memory bound if the estimate actually grows with the + // free-form `metadata` column. Returning a constant here (or ignoring nested strings) + // would silently restore the unbounded-by-entry-count behavior the weigher replaced. + let blob = "x".repeat(100_000); + let bare = record(None).approx_size_bytes(); + let nested = record(Some(json!({"git": {"commit_id": blob}}))).approx_size_bytes(); + + assert!( + nested >= bare + 100_000, + "nested metadata under-counted: {nested} vs {bare}" + ); + } +} diff --git a/rust/cymbal/src/modes/processing/app_context.rs b/rust/cymbal/src/modes/processing/app_context.rs index b4332752a16e..7a4ea53f6901 100644 --- a/rust/cymbal/src/modes/processing/app_context.rs +++ b/rust/cymbal/src/modes/processing/app_context.rs @@ -15,6 +15,7 @@ use crate::{ error::UnhandledError, modes::processing::config::{init_global_state, ProcessingConfig}, stages::rate_limiting::RedisRateLimiter, + stages::resolution::event_release::ReleaseCache, stages::resolution::remote::{ dns::TokioDnsResolver, pool::EndpointPool, resolver::RemoteResolutionContext, RemoteResolutionConfig, @@ -61,6 +62,10 @@ pub struct AppContext { // itself, so suppression / reopen always see current PG state (see `IssueLinker`). // moka caches are cheap to clone (internally Arc'd). pub issue_cache: Cache<(TeamId, String), Uuid>, + // Caches event-level release resolution (`$release_id` and the mobile app-metadata hash) so a + // per-event lookup doesn't re-hit Postgres for the same release, including the negative result + // for apps that never bound one. Lives here so it survives across batches. + pub release_cache: ReleaseCache, } impl Drop for AppContext { @@ -182,6 +187,11 @@ impl AppContext { .time_to_live(Duration::from_secs(config.issue_cache_ttl_seconds)) .build(); + let release_cache = ReleaseCache::new( + config.release_cache_max_bytes, + Duration::from_secs(config.release_cache_ttl_seconds), + ); + let (remote_resolution, remote_resolution_refresh_task) = build_remote_resolution(config).await?; @@ -205,6 +215,7 @@ impl AppContext { rate_limiter_enabled_team_ids, symbol_resolver, issue_cache, + release_cache, remote_resolution, remote_resolution_refresh_task, }) diff --git a/rust/cymbal/src/modes/processing/config.rs b/rust/cymbal/src/modes/processing/config.rs index b2ef3abeef46..2d1369656851 100644 --- a/rust/cymbal/src/modes/processing/config.rs +++ b/rust/cymbal/src/modes/processing/config.rs @@ -79,6 +79,17 @@ pub struct ProcessingConfig { #[envconfig(default = "100000")] pub issue_cache_capacity: u64, + // Event-level release resolution runs once per exception event. A release row is immutable + // once the CLI creates it, so a positive hit never goes stale; the TTL exists to let a + // negative result (app metadata that matches no release yet) expire after a dSYM upload + // creates the release, without re-querying Postgres on every event in the meantime. + #[envconfig(default = "300")] + pub release_cache_ttl_seconds: u64, + + // Bounded in bytes in case someone tries to do something funny. + #[envconfig(default = "33554432")] // 32 MiB + pub release_cache_max_bytes: u64, + // Maximum number of in-flight futures for a single `Batch::apply_func` call. // This is a per-call-site limit, not a global pipeline-wide concurrency cap. #[envconfig(default = "64")] diff --git a/rust/cymbal/src/modes/processing/stages/rate_limiting/mod.rs b/rust/cymbal/src/modes/processing/stages/rate_limiting/mod.rs index fc6e1a500a2c..86ea65cd36f4 100644 --- a/rust/cymbal/src/modes/processing/stages/rate_limiting/mod.rs +++ b/rust/cymbal/src/modes/processing/stages/rate_limiting/mod.rs @@ -671,6 +671,7 @@ mod tests { messages: vec![], functions: vec![], handled: false, + release: None, }, fingerprint: SelectedFingerprint::manual("test-fingerprint".to_string()), issue: Issue { diff --git a/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs new file mode 100644 index 000000000000..9395842fc966 --- /dev/null +++ b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs @@ -0,0 +1,253 @@ +use std::{collections::HashMap, sync::Arc, time::Duration}; + +use moka::future::{Cache, CacheBuilder}; +use serde_json::Value; +use sqlx::{Executor, Postgres}; +use uuid::Uuid; + +use crate::{ + error::UnhandledError, + frames::releases::{mobile_release_hash_id, ReleaseRecord}, + metric_consts::{ANCILLARY_CACHE, EVENT_RELEASE_RESOLVER_OPERATOR}, + stages::{pipeline::HandledError, resolution::ResolutionStage}, + types::{ + exception_event::{ExceptionEvent, Parsed}, + operator::{OperatorResult, TeamId, ValueOperator}, + }, +}; + +/// Per-worker cache for event-level release resolution. Both lookups run once per exception event +/// on the ingestion hot path, so without this a mobile app that never bound a release re-queries +/// Postgres on every event it sends (the common negative case). A release row is immutable once +/// the CLI creates it, so a positive hit never goes stale; caching the negative result too is what +/// removes the per-event query, and the TTL bounds how long a miss lingers after a later dSYM +/// upload creates the release. +/// +/// The two lookups key on different things — a release-row id for web builds, a reconstructed +/// content hash for mobile builds — so they get separate caches. `try_get_with` coalesces +/// concurrent misses for the same key, so a cold cache under load issues one query per key rather +/// than one per event. moka caches are internally Arc'd, so cloning this into each per-batch +/// `ResolutionStage` is cheap. +/// +/// Both caches are bounded in bytes rather than entries: a release carries a free-form `metadata` +/// JSON column that any client can write, so entry count says nothing about memory held. +#[derive(Clone)] +pub struct ReleaseCache { + by_id: Cache<(TeamId, Uuid), Option>, + by_hash: Cache<(TeamId, String), Option>, +} + +/// Charged on top of the payload for every entry, covering the key and moka's own per-entry +/// bookkeeping. It also keeps a negative entry (`None`) from weighing nothing: misses are the +/// high-cardinality side — one per app that never bound a release — so weightless negatives would +/// let the cache grow without bound, which is the whole thing the byte budget exists to stop. +const CACHE_ENTRY_OVERHEAD_BYTES: usize = 128; + +fn entry_weight(key_bytes: usize, value: &Option) -> u32 { + let bytes = CACHE_ENTRY_OVERHEAD_BYTES + + key_bytes + + value.as_ref().map_or(0, ReleaseRecord::approx_size_bytes); + bytes.try_into().unwrap_or(u32::MAX) +} + +impl ReleaseCache { + /// `max_bytes` bounds each of the two caches independently, so the pair can hold twice that. + pub fn new(max_bytes: u64, ttl: Duration) -> Self { + Self { + by_id: CacheBuilder::new(max_bytes) + .weigher(|_key, value: &Option| entry_weight(0, value)) + .time_to_live(ttl) + .build(), + by_hash: CacheBuilder::new(max_bytes) + .weigher(|key: &(TeamId, String), value: &Option| { + entry_weight(key.1.len(), value) + }) + .time_to_live(ttl) + .build(), + } + } + + /// A no-op cache for the paths that never resolve event releases (the remote resolution server + /// and frame-resolution tests). A zero byte budget retains nothing, but those paths never read + /// it anyway. + pub fn disabled() -> Self { + Self::new(0, Duration::from_secs(0)) + } + + async fn for_id<'c, E>( + &self, + e: E, + id: Uuid, + team_id: TeamId, + ) -> Result, UnhandledError> + where + E: Executor<'c, Database = Postgres>, + { + let mut cache_miss = false; + let record = self + .by_id + .try_get_with((team_id, id), async { + cache_miss = true; + ReleaseRecord::for_id(e, id, team_id).await + }) + .await + .map_err(|e: Arc| UnhandledError::Other(e.to_string()))?; + + record_cache_outcome("release_by_id", cache_miss); + Ok(record) + } + + async fn for_hash<'c, E>( + &self, + e: E, + hash_id: &str, + team_id: TeamId, + ) -> Result, UnhandledError> + where + E: Executor<'c, Database = Postgres>, + { + let mut cache_miss = false; + let record = self + .by_hash + .try_get_with((team_id, hash_id.to_string()), async { + cache_miss = true; + ReleaseRecord::for_hash(e, hash_id, team_id).await + }) + .await + .map_err(|e: Arc| UnhandledError::Other(e.to_string()))?; + + record_cache_outcome("release_by_hash", cache_miss); + Ok(record) + } +} + +fn record_cache_outcome(cache_type: &'static str, cache_miss: bool) { + let outcome = if cache_miss { "miss" } else { "hit" }; + metrics::counter!(ANCILLARY_CACHE, "type" => cache_type, "outcome" => outcome).increment(1); +} + +/// Resolves the event-level release without going through the per-frame symbol-set join, so the +/// release is independent of which chunks resolved the stack. +/// +/// Two sources, in order of preference: +/// 1. `$release_id` — web builds inject the release row's id, which the SDK emits verbatim. Direct +/// foreign-key lookup. +/// 2. app metadata — mobile SDKs inject nothing, but every event already carries `$app_namespace`, +/// `$app_version`, and `$app_build`, which the CLI hashed into the release when it uploaded the +/// dSYMs. We reconstruct that hash and look the release up by it. +/// +/// When neither resolves, the event release stays unset and the pipeline falls back to the +/// per-frame symbol-set join for legacy events. +#[derive(Clone, Default)] +pub struct EventReleaseResolver; + +impl ValueOperator for EventReleaseResolver { + type Context = ResolutionStage; + type Item = ExceptionEvent; + type HandledError = HandledError; + type UnhandledError = UnhandledError; + + fn name(&self) -> &'static str { + EVENT_RELEASE_RESOLVER_OPERATOR + } + + async fn execute_value( + &self, + mut evt: ExceptionEvent, + ctx: ResolutionStage, + ) -> OperatorResult { + // No pool means the remote resolution server, which never resolves event releases. + let Some(pool) = ctx.posthog_pool.as_ref() else { + return Ok(Ok(evt)); + }; + + let release_id = evt + .properties() + .get("$release_id") + .and_then(Value::as_str) + .and_then(|id| Uuid::parse_str(id).ok()); + + if let Some(release_id) = release_id { + let record = ctx + .release_cache + .for_id(pool, release_id, evt.team_id()) + .await?; + evt.set_event_release(record); + } else if let Some(hash_id) = mobile_release_hash_from_props(evt.properties()) { + let record = ctx + .release_cache + .for_hash(pool, &hash_id, evt.team_id()) + .await?; + evt.set_event_release(record); + } + + Ok(Ok(evt)) + } +} + +/// Rebuild the release `hash_id` from a mobile event's app metadata. Returns `None` for events that +/// aren't from a mobile SDK (no `$app_namespace`) or lack any version info to key on. +/// +/// `$app_build` arrives as a JSON number when the SDK parsed `CFBundleVersion` as an integer, so +/// accept a number or a string and render it the same way the CLI saw the raw plist value. +fn mobile_release_hash_from_props(props: &HashMap) -> Option { + let namespace = props.get("$app_namespace").and_then(Value::as_str)?; + let version = props.get("$app_version").and_then(Value::as_str); + let build = props.get("$app_build").and_then(scalar_to_string); + mobile_release_hash_id(namespace, version, build.as_deref()) +} + +fn scalar_to_string(value: &Value) -> Option { + match value { + Value::String(s) => Some(s.clone()), + Value::Number(n) => Some(n.to_string()), + _ => None, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn props(value: Value) -> HashMap { + value + .as_object() + .unwrap() + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect() + } + + #[test] + fn negative_entries_are_not_weightless() { + // Misses are the high-cardinality side of this cache — one per app that never bound a + // release — so a zero-weight `None` would let the by-hash cache grow without bound under + // the byte budget, which is exactly what the weigher exists to prevent. + assert!(entry_weight(0, &None) > 0); + assert!(entry_weight(128, &None) > entry_weight(0, &None)); + } + + #[test] + fn numeric_and_string_build_hash_identically() { + // The iOS SDK parses a numeric CFBundleVersion into an Int, so `$app_build` arrives as a JSON + // number. It must hash the same as its string form, since the CLI hashed the raw plist string. + let from_number = mobile_release_hash_from_props(&props(json!({ + "$app_namespace": "com.app", "$app_version": "1.0", "$app_build": 1 + }))); + let from_string = mobile_release_hash_from_props(&props(json!({ + "$app_namespace": "com.app", "$app_version": "1.0", "$app_build": "1" + }))); + assert!(from_number.is_some()); + assert_eq!(from_number, from_string); + } + + #[test] + fn non_mobile_event_yields_no_hash() { + // No `$app_namespace` means it isn't a mobile SDK event, so there's nothing to resolve. + assert_eq!( + mobile_release_hash_from_props(&props(json!({"$app_version": "1.0"}))), + None + ); + } +} diff --git a/rust/cymbal/src/modes/processing/stages/resolution/frame.rs b/rust/cymbal/src/modes/processing/stages/resolution/frame.rs index 45238a297f63..fe93e9e97063 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/frame.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/frame.rs @@ -236,6 +236,7 @@ mod test { zip_fixture, }, modes::processing::config::ProcessingConfig, + stages::resolution::event_release::ReleaseCache, symbolication::symbol::local::LocalSymbolResolver, symbolication::symbol_store::Catalog, }; @@ -348,6 +349,8 @@ mod test { db.clone(), )), symbol_resolution_limiter: Arc::new(Semaphore::new(4)), + posthog_pool: Some(db.clone()), + release_cache: ReleaseCache::disabled(), remote: None, } } diff --git a/rust/cymbal/src/modes/processing/stages/resolution/mod.rs b/rust/cymbal/src/modes/processing/stages/resolution/mod.rs index 0558844d73d1..e15960e5e165 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/mod.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/mod.rs @@ -1,7 +1,9 @@ use std::sync::Arc; +use sqlx::PgPool; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; +pub mod event_release; pub mod exception; pub mod frame; pub mod legacy; @@ -13,6 +15,7 @@ use crate::{ metric_consts::RESOLUTION_STAGE, stages::pipeline::{ParsedPipelineItem, ResolvedPipelineItem}, stages::resolution::{ + event_release::{EventReleaseResolver, ReleaseCache}, exception::ExceptionResolver, frame::FrameResolver, legacy::LegacyOrderResolver, @@ -29,6 +32,13 @@ use crate::{ pub struct ResolutionStage { pub symbol_resolver: Arc, pub symbol_resolution_limiter: Arc, + /// Used to resolve the event-level release id (`$release_id`) to its release row. `None` on the + /// remote resolution server, which only symbolicates frames and never resolves event releases. + pub posthog_pool: Option, + /// Caches event-level release lookups (`$release_id` and mobile app-metadata hash) so the + /// per-event resolution doesn't re-hit Postgres for the same release. Lives on `AppContext`, + /// cloned in here per batch. + pub release_cache: ReleaseCache, /// When `Some`, the resolution stage can route sampled events through the /// remote `cymbal.resolution.v1` client path. Unsampled events still use /// local exception+frame resolution. There is no local fallback for events @@ -42,6 +52,8 @@ impl From<&Arc> for ResolutionStage { Self { symbol_resolver: app_context.as_ref().symbol_resolver.clone(), symbol_resolution_limiter: app_context.as_ref().symbol_resolution_limiter.clone(), + posthog_pool: Some(app_context.as_ref().posthog_pool.clone()), + release_cache: app_context.as_ref().release_cache.clone(), remote: app_context.as_ref().remote_resolution.clone(), } } @@ -77,6 +89,8 @@ impl Stage for ResolutionStage { let resolved = resolve_batch(batch, remote, self.clone()) .await? .apply_operator(LegacyOrderResolver, self.clone()) + .await? + .apply_operator(EventReleaseResolver, self.clone()) .await?; return Ok(resolved.map(|item, ()| item.map(|event| event.into_resolved()), &mut ())); } @@ -87,6 +101,8 @@ impl Stage for ResolutionStage { .apply_operator(FrameResolver, self.clone()) .await? .apply_operator(LegacyOrderResolver, self.clone()) + .await? + .apply_operator(EventReleaseResolver, self.clone()) .await?; Ok(resolved.map(|item, ()| item.map(|event| event.into_resolved()), &mut ())) } diff --git a/rust/cymbal/src/modes/processing/types/exception_event.rs b/rust/cymbal/src/modes/processing/types/exception_event.rs index 45e6a8c12eb7..db31b2239e67 100644 --- a/rust/cymbal/src/modes/processing/types/exception_event.rs +++ b/rust/cymbal/src/modes/processing/types/exception_event.rs @@ -7,6 +7,7 @@ use uuid::Uuid; use crate::{ error::EventError, fingerprinting::{Fingerprint, FingerprintRecordPart, FingerprintVersion}, + frames::releases::{ReleaseInfo, ReleaseRecord}, issue_resolution::Issue, langs::native::DebugImage, modes::processing::normalization::normalize_wire_order, @@ -25,6 +26,9 @@ pub struct Parsed { pub(crate) client_fingerprint: Option, pub(crate) legacy_order_exception_list: Option, pub(crate) legacy_order_resolved: Option, + /// The release resolved from the event's `$release_id` or mobile app metadata, if any. Set by + /// `EventReleaseResolver` and emitted as `$exception_release` at `into_resolved`. + pub(crate) event_release: Option, } #[derive(Debug, Clone)] @@ -34,16 +38,23 @@ pub struct ResolvedMetadata { pub messages: Vec, pub functions: Vec, pub handled: bool, + /// The single release the event resolves to, from its `$release_id` or mobile app metadata. + /// Emitted as `$exception_release`. + pub release: Option, } impl ResolvedMetadata { - fn from_exception_list(exception_list: &ExceptionList) -> Self { + fn from_exception_list( + exception_list: &ExceptionList, + event_release: Option<&ReleaseRecord>, + ) -> Self { Self { sources: exception_list.get_unique_sources(), types: exception_list.get_unique_types(), messages: exception_list.get_unique_messages(), functions: exception_list.get_unique_functions(), handled: exception_list.get_is_handled(), + release: event_release.map(ReleaseRecord::to_info), } } } @@ -202,8 +213,15 @@ impl ExceptionEvent { self.state.legacy_order_resolved = Some(exception_list); } + pub(crate) fn set_event_release(&mut self, release: Option) { + self.state.event_release = release; + } + pub(crate) fn into_resolved(self) -> ExceptionEvent { - let metadata = ResolvedMetadata::from_exception_list(&self.exception_list); + let metadata = ResolvedMetadata::from_exception_list( + &self.exception_list, + self.state.event_release.as_ref(), + ); self.map_state(|state| Resolved { metadata, client_fingerprint: state.client_fingerprint, @@ -353,6 +371,12 @@ impl ExceptionEvent { serde_json::to_value(metadata.functions).expect("exception functions are serializable"), ); map.insert("$exception_handled".into(), Value::Bool(metadata.handled)); + if let Some(release) = metadata.release { + map.insert( + "$exception_release".into(), + serde_json::to_value(release).expect("exception release is serializable"), + ); + } map.insert( "$exception_fingerprint".into(), Value::String(fingerprint.value), @@ -413,6 +437,12 @@ impl ExceptionEvent { .expect("exception functions are serializable"), ); map.insert("$exception_handled".into(), Value::Bool(metadata.handled)); + if let Some(release) = &metadata.release { + map.insert( + "$exception_release".into(), + serde_json::to_value(release).expect("exception release is serializable"), + ); + } if let Some(name) = &self.proposed_issue_name { map.insert("$issue_name".into(), Value::String(name.clone())); } @@ -474,6 +504,7 @@ impl ExceptionEvent { issue_id, other: self.props.clone(), handled: metadata.handled, + release: metadata.release.clone(), types: metadata.types.clone(), values: metadata.messages.clone(), sources: metadata.sources.clone(), @@ -524,6 +555,7 @@ impl TryFrom for ExceptionEvent { "$exception_types", "$exception_values", "$exception_functions", + "$exception_release", "$exception_fingerprint_version", "$exception_proposed_fingerprint", "$exception_fingerprint_record", @@ -550,6 +582,7 @@ impl TryFrom for ExceptionEvent { client_fingerprint: raw.fingerprint, legacy_order_exception_list, legacy_order_resolved: None, + event_release: None, }, }) } @@ -600,6 +633,7 @@ mod tests { messages: vec!["boom".to_string()], functions: vec![], handled: false, + release: None, }, client_fingerprint: Some("client-fingerprint".to_string()), legacy_order_resolved: None, @@ -677,4 +711,64 @@ mod tests { assert_eq!(rate_limit["$exception_issue_id"], issue.id.to_string()); assert_eq!(rate_limit["passthrough"], true); } + + fn release_record(hash_id: &str) -> ReleaseRecord { + ReleaseRecord { + id: Uuid::now_v7(), + team_id: 42, + hash_id: hash_id.to_string(), + created_at: chrono::DateTime::from_timestamp(0, 0).unwrap(), + version: "1.2.3".to_string(), + project: "my-app".to_string(), + metadata: None, + } + } + + #[test] + fn event_release_populates_the_singular_release() { + // The event-level release (from `$release_id` or mobile app metadata) is the sole source of + // `$exception_release`; it does not depend on any frame carrying a release. + let metadata = ResolvedMetadata::from_exception_list( + &ExceptionList::default(), + Some(&release_record("hash-abc")), + ); + assert!(metadata.release.is_some()); + } + + #[test] + fn missing_event_release_leaves_the_release_unset() { + // Without an event-level release there is nothing to emit; there is no per-frame fallback. + let metadata = ResolvedMetadata::from_exception_list(&ExceptionList::default(), None); + assert!(metadata.release.is_none()); + } + + #[test] + fn exception_release_emitted_only_when_a_release_resolves() { + let issue = Issue { + id: Uuid::now_v7(), + team_id: 42, + status: crate::issue_resolution::IssueStatus::Active, + name: None, + description: None, + created_at: chrono::Utc::now(), + }; + let expected = serde_json::to_value(release_record("hash-abc").to_info()).unwrap(); + + // A resolved release surfaces as `$exception_release` on both the grouping-rule projection + // and the derived wire form. + let mut resolved = resolved_event(); + resolved.state.metadata.release = Some(release_record("hash-abc").to_info()); + let grouping = resolved.grouping_rule_properties(); + assert_eq!(grouping["$exception_release"], expected); + let fingerprinted = + resolved.into_fingerprinted(SelectedFingerprint::manual("fp".to_string())); + let wire = serde_json::to_value(fingerprinted.processed_properties(&issue)).unwrap(); + assert_eq!(wire["$exception_release"], expected); + + // No release resolved: the property is omitted. + let mut resolved = resolved_event(); + resolved.state.metadata.release = None; + let grouping = resolved.grouping_rule_properties(); + assert!(grouping.get("$exception_release").is_none()); + } } diff --git a/rust/cymbal/src/modes/processing/types/mod.rs b/rust/cymbal/src/modes/processing/types/mod.rs index a4fef225a359..05a4caca9fe0 100644 --- a/rust/cymbal/src/modes/processing/types/mod.rs +++ b/rust/cymbal/src/modes/processing/types/mod.rs @@ -7,6 +7,7 @@ use std::ops::{Deref, DerefMut}; use uuid::Uuid; use crate::fingerprinting::{FingerprintRecordPart, FingerprintVersion}; +use crate::frames::releases::ReleaseInfo; use crate::frames::{Frame, RawFrame}; use crate::langs::native::DebugImage; use crate::metric_consts::POSTHOG_SDK_EXCEPTION_RESOLVED; @@ -149,6 +150,13 @@ struct ProcessedExceptionPropertiesWire { other: HashMap, #[serde(rename = "$exception_handled")] handled: bool, + // The single release an event resolves to, from its `$release_id` or mobile app metadata. + #[serde( + rename = "$exception_release", + skip_serializing_if = "Option::is_none", + default + )] + release: Option, #[serde(rename = "$exception_types")] types: Vec, #[serde(rename = "$exception_values")] diff --git a/rust/cymbal/src/modes/resolution/service.rs b/rust/cymbal/src/modes/resolution/service.rs index a6c87fd0fef3..bfc36ec12c2e 100644 --- a/rust/cymbal/src/modes/resolution/service.rs +++ b/rust/cymbal/src/modes/resolution/service.rs @@ -3,6 +3,7 @@ use std::sync::{ Arc, }; +use crate::stages::resolution::event_release::ReleaseCache; use crate::stages::resolution::ResolutionStage; use crate::symbolication::symbol::SymbolResolver; use std::pin::Pin; @@ -65,6 +66,10 @@ impl CymbalResolutionService { ResolutionStage { symbol_resolver: self.symbol_resolver.clone(), symbol_resolution_limiter: self.symbol_resolution_limiter.clone(), + // The resolution server only symbolicates frames; event-level release resolution runs + // on the processing side, so no release pool or cache is needed here. + posthog_pool: None, + release_cache: ReleaseCache::disabled(), // The cymbal-resolution server never enables remote mode itself; // it is the server side that cymbal talks to. Local resolution is // the only valid path here. diff --git a/rust/cymbal/tests/common/mod.rs b/rust/cymbal/tests/common/mod.rs index 56e5a05fa989..4d55ebf8c099 100644 --- a/rust/cymbal/tests/common/mod.rs +++ b/rust/cymbal/tests/common/mod.rs @@ -21,6 +21,7 @@ use cymbal::frames::{Frame, RawFrame}; use cymbal::langs::native::DebugImage; use cymbal::stages::pipeline::ParsedPipelineItem; use cymbal::stages::resolution::{ + event_release::ReleaseCache, remote::{ config::RemoteResolutionConfig, pool::EndpointPool, resolver::RemoteResolutionContext, }, @@ -451,6 +452,8 @@ pub fn remote_stage(ctx: RemoteResolutionContext) -> ResolutionStage { ResolutionStage { symbol_resolver: Arc::new(NoopResolver), symbol_resolution_limiter: Arc::new(Semaphore::new(4)), + posthog_pool: None, + release_cache: ReleaseCache::disabled(), remote: Some(ctx), } } @@ -459,6 +462,8 @@ pub fn local_stage() -> ResolutionStage { ResolutionStage { symbol_resolver: Arc::new(NoopResolver), symbol_resolution_limiter: Arc::new(Semaphore::new(4)), + posthog_pool: None, + release_cache: ReleaseCache::disabled(), remote: None, } } diff --git a/rust/cymbal/tests/remote_resolution.rs b/rust/cymbal/tests/remote_resolution.rs index f798a53c83af..857c2469f5d1 100644 --- a/rust/cymbal/tests/remote_resolution.rs +++ b/rust/cymbal/tests/remote_resolution.rs @@ -21,6 +21,7 @@ use common::{ use cymbal::error::{ResolveError, UnhandledError}; use cymbal::frames::{Frame, RawFrame}; use cymbal::langs::native::DebugImage; +use cymbal::stages::resolution::event_release::ReleaseCache; use cymbal::stages::resolution::ResolutionStage; use cymbal::symbolication::symbol::SymbolResolver; use cymbal::symbolication::symbol_store::chunk_id::OrChunkId; @@ -81,6 +82,8 @@ fn remote_stage_with_resolver( ResolutionStage { symbol_resolver: resolver, symbol_resolution_limiter: Arc::new(Semaphore::new(4)), + posthog_pool: None, + release_cache: ReleaseCache::disabled(), remote: Some(ctx), } } diff --git a/rust/cymbal/tests/remote_resolution_parity.rs b/rust/cymbal/tests/remote_resolution_parity.rs index 7281ce8f11d0..d6afa24b6704 100644 --- a/rust/cymbal/tests/remote_resolution_parity.rs +++ b/rust/cymbal/tests/remote_resolution_parity.rs @@ -25,6 +25,7 @@ use cymbal::frames::{Frame, RawFrame}; use cymbal::langs::native::DebugImage; use cymbal::modes::resolution::load_monitor::LoadMonitor; use cymbal::modes::resolution::service::{CymbalResolutionService, ServiceConfig}; +use cymbal::stages::resolution::event_release::ReleaseCache; use cymbal::stages::resolution::ResolutionStage; use cymbal::symbolication::symbol::SymbolResolver; use cymbal::symbolication::symbol_store::chunk_id::OrChunkId; @@ -112,6 +113,8 @@ fn local_stage(resolver: Arc) -> ResolutionStage { ResolutionStage { symbol_resolver: resolver, symbol_resolution_limiter: Arc::new(Semaphore::new(4)), + posthog_pool: None, + release_cache: ReleaseCache::disabled(), remote: None, } } @@ -126,6 +129,8 @@ fn remote_stage( // local fallback would still produce parity-matching output. symbol_resolver: resolver, symbol_resolution_limiter: Arc::new(Semaphore::new(4)), + posthog_pool: None, + release_cache: ReleaseCache::disabled(), remote: Some(remote), } } From 0ba8f76e4a03b24992efd51e7f36848c751fa06d Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Sun, 2 Aug 2026 12:16:04 +0200 Subject: [PATCH 2/9] feat(error-tracking): return frame releases over the resolution wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #75456 removed the frame-derived release enrichment because it could not survive remote symbol resolution: Frame.release is #[serde(skip)] (the frame JSON shape doubles as clickhouse output), so the release the symbol-set join attached was dropped when the resolved exception was serialized back to the processing worker. With 100% of resolution traffic on cymbal-resolution, that made the whole path dead code. This brings the join back and fixes the transport instead of the field: - Restore the symbol-set→release join (for_symbol_set_ref on fresh resolves, for_symbol_set_id on PG frame-cache loads) and the in-memory Frame.release carrier, verbatim from the pre-#75456 state. - Add Done.releases_json to cymbal.resolution.v1: a JSON-array sidecar of the releases bound to the symbol sets that resolved the item's frames, deduped by release id. Additive proto3 field, so both deploy skew directions degrade to today's behavior: an older server sends empty bytes (treated as no releases), an older client ignores the field. The exception JSON shape is untouched. - Processing accumulates the sidecar per event (local, unsampled resolution reads Frame.release directly) and, only when the event-level resolver ($release_id, mobile app-metadata hash) found nothing, emits the latest release by created_at as $exception_release. Event-level resolution keeps precedence. Parity gets an explicit test: the byte-for-byte exception-list comparison cannot see any of this (the field is serde-skipped), so a release-attaching fake resolver now asserts both paths produce the same $exception_release. Co-Authored-By: Claude Fable 5 --- proto/cymbal/resolution/v1/resolution.proto | 5 + rust/cymbal-proto/tests/contract.rs | 25 +++- ...e71e76ff7b6595c2953ed108da0bd0f2279b9.json | 59 ++++++++ ...5fcb2bbd74c80b293672c4579531521e905cd.json | 59 ++++++++ rust/cymbal/docs/compatibility.md | 1 + .../src/core/symbolication/symbol/local.rs | 23 +++- .../src/core/symbolication/symbol/records.rs | 9 +- rust/cymbal/src/core/types/frames/mod.rs | 6 + rust/cymbal/src/core/types/frames/releases.rs | 78 ++++++++++- rust/cymbal/src/core/types/langs/apple.rs | 3 + rust/cymbal/src/core/types/langs/custom.rs | 1 + rust/cymbal/src/core/types/langs/dart.rs | 1 + rust/cymbal/src/core/types/langs/go.rs | 1 + rust/cymbal/src/core/types/langs/hermes.rs | 3 + rust/cymbal/src/core/types/langs/java.rs | 2 + rust/cymbal/src/core/types/langs/js.rs | 3 + rust/cymbal/src/core/types/langs/native.rs | 3 + rust/cymbal/src/core/types/langs/node.rs | 3 + rust/cymbal/src/core/types/langs/php.rs | 1 + rust/cymbal/src/core/types/langs/python.rs | 1 + rust/cymbal/src/core/types/langs/ruby.rs | 1 + .../modes/processing/fingerprinting/mod.rs | 1 + .../src/modes/processing/normalization.rs | 1 + .../stages/resolution/remote/mux.rs | 1 + .../stages/resolution/remote/resolver.rs | 5 + .../resolution/remote/resolver/retry.rs | 83 +++++++++++- .../modes/processing/types/exception_event.rs | 127 +++++++++++++++++- rust/cymbal/src/modes/processing/types/mod.rs | 9 +- rust/cymbal/src/modes/resolution/README.md | 2 +- rust/cymbal/src/modes/resolution/service.rs | 6 +- .../src/modes/resolution/service/resolve.rs | 43 +++++- rust/cymbal/tests/common/mod.rs | 15 ++- rust/cymbal/tests/event.rs | 2 + rust/cymbal/tests/fingerprint_golden.rs | 1 + rust/cymbal/tests/remote_resolution_parity.rs | 120 ++++++++++++++++- rust/cymbal/tests/resolution_service_tests.rs | 73 ++++++++++ 36 files changed, 744 insertions(+), 33 deletions(-) create mode 100644 rust/cymbal/.sqlx/query-7002c21150e6a6e42b57cb9ebf9e71e76ff7b6595c2953ed108da0bd0f2279b9.json create mode 100644 rust/cymbal/.sqlx/query-eb812dd93e8af10192d414fb1d55fcb2bbd74c80b293672c4579531521e905cd.json diff --git a/proto/cymbal/resolution/v1/resolution.proto b/proto/cymbal/resolution/v1/resolution.proto index 450e12c861a1..265c2c5d6cc7 100644 --- a/proto/cymbal/resolution/v1/resolution.proto +++ b/proto/cymbal/resolution/v1/resolution.proto @@ -60,6 +60,11 @@ message Done { // remapping. It replaces only the submitted exception item, not the full // event or exception list. bytes resolved_exception_json = 1; + // Serialized JSON array of the releases bound to the symbol sets that + // resolved this exception's frames, deduped by release id. Empty when no + // resolved frame had a release. Callers must tolerate empty bytes (older + // servers do not set this field). + bytes releases_json = 2; } // Error is a terminal item failure. The kind enum is the shared control-flow diff --git a/rust/cymbal-proto/tests/contract.rs b/rust/cymbal-proto/tests/contract.rs index 17aea85b8db3..c3c54480e9a0 100644 --- a/rust/cymbal-proto/tests/contract.rs +++ b/rust/cymbal-proto/tests/contract.rs @@ -30,6 +30,7 @@ fn resolve_outcome_echoes_id_and_carries_done_error_or_retry() { id: 1, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: br#"{"type":"ResolvedError"}"#.to_vec(), + releases_json: br#"[{"version":"1.2.3"}]"#.to_vec(), })), }, ResolveOutcome { @@ -68,8 +69,9 @@ fn resolve_outcome_echoes_id_and_carries_done_error_or_retry() { .collect(); assert!(matches!( - decoded[0].result, - Some(resolve_outcome::Result::Done(_)) + &decoded[0].result, + Some(resolve_outcome::Result::Done(done)) + if done.releases_json == br#"[{"version":"1.2.3"}]"# )); assert!(matches!( decoded[1].result, @@ -99,6 +101,25 @@ fn resolve_outcome_echoes_id_and_carries_done_error_or_retry() { ); } +#[test] +fn done_without_releases_field_decodes_to_empty_bytes() { + // Skew compatibility both ways: a Done with no releases encodes byte-identical to the + // pre-`releases_json` message (prost omits default fields), and decoding such a message — + // what an older server sends — yields empty bytes, which callers must treat as "no releases". + let without_releases = Done { + resolved_exception_json: br#"{"type":"ResolvedError"}"#.to_vec(), + releases_json: Vec::new(), + }; + + let decoded = Done::decode(without_releases.encode_to_vec().as_slice()).unwrap(); + + assert_eq!( + decoded.resolved_exception_json, + br#"{"type":"ResolvedError"}"# + ); + assert!(decoded.releases_json.is_empty()); +} + #[test] fn subscribe_request_round_trips_caller_hint_and_identity() { let request = SubscribeRequest { diff --git a/rust/cymbal/.sqlx/query-7002c21150e6a6e42b57cb9ebf9e71e76ff7b6595c2953ed108da0bd0f2279b9.json b/rust/cymbal/.sqlx/query-7002c21150e6a6e42b57cb9ebf9e71e76ff7b6595c2953ed108da0bd0f2279b9.json new file mode 100644 index 000000000000..dbd7664b1090 --- /dev/null +++ b/rust/cymbal/.sqlx/query-7002c21150e6a6e42b57cb9ebf9e71e76ff7b6595c2953ed108da0bd0f2279b9.json @@ -0,0 +1,59 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT r.id, r.team_id, r.hash_id, r.created_at, r.version, r.project, r.metadata\n FROM posthog_errortrackingsymbolset ss\n INNER JOIN posthog_errortrackingrelease r ON ss.release_id = r.id\n WHERE ss.ref = $1 AND ss.team_id = $2\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Uuid" + }, + { + "ordinal": 1, + "name": "team_id", + "type_info": "Int4" + }, + { + "ordinal": 2, + "name": "hash_id", + "type_info": "Text" + }, + { + "ordinal": 3, + "name": "created_at", + "type_info": "Timestamptz" + }, + { + "ordinal": 4, + "name": "version", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "project", + "type_info": "Text" + }, + { + "ordinal": 6, + "name": "metadata", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Text", + "Int4" + ] + }, + "nullable": [ + false, + false, + false, + false, + false, + false, + true + ] + }, + "hash": "7002c21150e6a6e42b57cb9ebf9e71e76ff7b6595c2953ed108da0bd0f2279b9" +} diff --git a/rust/cymbal/.sqlx/query-eb812dd93e8af10192d414fb1d55fcb2bbd74c80b293672c4579531521e905cd.json b/rust/cymbal/.sqlx/query-eb812dd93e8af10192d414fb1d55fcb2bbd74c80b293672c4579531521e905cd.json new file mode 100644 index 000000000000..9f769295c356 --- /dev/null +++ b/rust/cymbal/.sqlx/query-eb812dd93e8af10192d414fb1d55fcb2bbd74c80b293672c4579531521e905cd.json @@ -0,0 +1,59 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT r.id, r.team_id, r.hash_id, r.created_at, r.version, r.project, r.metadata\n FROM posthog_errortrackingsymbolset ss\n INNER JOIN posthog_errortrackingrelease r ON ss.release_id = r.id\n WHERE ss.id = $1 AND ss.team_id = $2\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Uuid" + }, + { + "ordinal": 1, + "name": "team_id", + "type_info": "Int4" + }, + { + "ordinal": 2, + "name": "hash_id", + "type_info": "Text" + }, + { + "ordinal": 3, + "name": "created_at", + "type_info": "Timestamptz" + }, + { + "ordinal": 4, + "name": "version", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "project", + "type_info": "Text" + }, + { + "ordinal": 6, + "name": "metadata", + "type_info": "Jsonb" + } + ], + "parameters": { + "Left": [ + "Uuid", + "Int4" + ] + }, + "nullable": [ + false, + false, + false, + false, + false, + false, + true + ] + }, + "hash": "eb812dd93e8af10192d414fb1d55fcb2bbd74c80b293672c4579531521e905cd" +} diff --git a/rust/cymbal/docs/compatibility.md b/rust/cymbal/docs/compatibility.md index f8fdf51b5f83..87109dc68771 100644 --- a/rust/cymbal/docs/compatibility.md +++ b/rust/cymbal/docs/compatibility.md @@ -29,6 +29,7 @@ No generated type changes are needed: - Unsampled events use the local exception and frame resolvers. - Sampled remote events are flattened into exception-level `ResolveItem`s. Items are grouped by their first symbol-set reference when available, with a per-team fallback, then submitted over per-endpoint bidirectional `Resolve` streams. - Resolver-specific context is carried in `ResolveItem.metadata` as JSON bytes. The native symbolication convention uses a `debug_images_json` key. +- `Done.releases_json` carries the releases bound to the symbol sets that resolved the item's frames, as a JSON array deduped by release id. Empty bytes means no releases (also what servers predating the field send); `Frame.release` itself is `#[serde(skip)]` and never crosses the wire. - Per-item `ResolveOutcome.Error.kind` is the control-flow surface. `ERROR_KIND_OVERLOADED` is result-only backpressure and triggers item reroute. Accepted items emit `ResolveOutcome.Accepted` before their terminal outcome; cymbal releases its routing permit on that acceptance signal. If `CYMBAL_REMOTE_RESOLUTION_OVERLOAD_EJECTION_MS` is non-zero, the overloaded endpoint is also temporarily excluded from new routing in that cymbal process. Repeated overloads double that cooldown up to `CYMBAL_REMOTE_RESOLUTION_OVERLOAD_EJECTION_MAX_MS`, and a quiet `CYMBAL_REMOTE_RESOLUTION_OVERLOAD_EJECTION_DECAY_MS` window resets it. `CYMBAL_REMOTE_RESOLUTION_ROUTING_JITTER` controls how much routing flattens across the load-adjusted rendezvous-ranked candidate list (`0.0` strict top load-adjusted candidate, `1.0` load-weighted across candidates). `LoadEvent` carries endpoint freshness/draining state plus `in_flight` / `max_in_flight` as a soft routing load signal. This means Node request chunking limits protect cymbal's public HTTP body size, while cymbal's private gRPC path owns exception-level routing, reroute depth, and overload handling. diff --git a/rust/cymbal/src/core/symbolication/symbol/local.rs b/rust/cymbal/src/core/symbolication/symbol/local.rs index 29a35487ae3c..029bec3eec94 100644 --- a/rust/cymbal/src/core/symbolication/symbol/local.rs +++ b/rust/cymbal/src/core/symbolication/symbol/local.rs @@ -16,7 +16,7 @@ use sqlx::PgPool; use crate::{ core::config::ResolverConfig, error::{JsResolveErr, ProguardError, ResolveError, UnhandledError}, - frames::{Frame, RawFrame}, + frames::{releases::ReleaseRecord, Frame, RawFrame}, langs::native::DebugImage, metric_consts::{ FRAME_CACHE_HITS, FRAME_CACHE_MISSES, FRAME_DB_HITS, FRAME_DB_MISSES, @@ -156,19 +156,28 @@ impl LocalSymbolResolver { assert!(!resolved.is_empty()); // If this ever happens, we've got a data-dropping bug, and want to crash - let set = if let Some(set_ref) = frame.symbol_set_ref(debug_images) { - let mut set = SymbolSetRecord::load(&self.pool, raw_id.team_id, &set_ref).await?; + let (set, release) = if let Some(set_ref) = frame.symbol_set_ref(debug_images) { + let set_fut = SymbolSetRecord::load(&self.pool, raw_id.team_id, &set_ref); + let release_fut = async { + ReleaseRecord::for_symbol_set_ref(&self.pool, &set_ref, raw_id.team_id) + .await + .map_err(UnhandledError::from) + }; + let (mut set, release) = tokio::try_join!(set_fut, release_fut)?; if let Some(s) = &mut set { s.set_last_used(&self.pool).await?; } - set + (set, release) } else { - None + (None, None) }; let mut records = Vec::new(); - for r_frame in &resolved { - // Save back to the DB + let mut resolved = resolved; + for r_frame in resolved.iter_mut() { + r_frame.release = release.clone(); // Enrich with release information + + // And save back to the DB let record = ErrorTrackingStackFrame::new( r_frame.frame_id.clone(), set.as_ref().map(|s| s.id), diff --git a/rust/cymbal/src/core/symbolication/symbol/records.rs b/rust/cymbal/src/core/symbolication/symbol/records.rs index e53608af9dc9..0a5f3c5bf609 100644 --- a/rust/cymbal/src/core/symbolication/symbol/records.rs +++ b/rust/cymbal/src/core/symbolication/symbol/records.rs @@ -6,7 +6,7 @@ use sqlx::Executor; use uuid::Uuid; use crate::error::UnhandledError; -use crate::frames::{Context, Frame}; +use crate::frames::{releases::ReleaseRecord, Context, Frame}; const FRAME_TTL_JITTER_PERCENT: u32 = 10; @@ -175,6 +175,11 @@ impl ErrorTrackingStackFrame { return Ok(Vec::new()); } + let mut release = None; + if let Some(ss_id) = &res[0].symbol_set_id { + release = ReleaseRecord::for_symbol_set_id(e, *ss_id, id.team_id).await?; + } + for found in res { // Frame ID's lose team_id when they're serialized, so we fix that up here when loading them let frame_id = FrameId::new(found.raw_id, found.team_id, found.part); @@ -192,6 +197,7 @@ impl ErrorTrackingStackFrame { None }; + frame.release = release.clone(); frame.context = context.clone(); results.push(Self { @@ -230,6 +236,7 @@ mod tests { junk_drawer: None, code_variables: None, context: None, + release: None, } } diff --git a/rust/cymbal/src/core/types/frames/mod.rs b/rust/cymbal/src/core/types/frames/mod.rs index 5b06b43a60f4..ce35c6401c4b 100644 --- a/rust/cymbal/src/core/types/frames/mod.rs +++ b/rust/cymbal/src/core/types/frames/mod.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use common_types::error_tracking::{FrameData, FrameId, RawFrameId}; +use releases::ReleaseRecord; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -172,6 +173,11 @@ pub struct Frame { // use in the frontend #[serde(skip)] pub context: Option, + // The release bound to the symbol set that resolved this frame. Never serialized: it must not + // reach the clickhouse-bound event JSON, and the remote resolution response carries releases + // in its own `releases_json` sidecar field instead of inside the frame. + #[serde(skip)] + pub release: Option, } #[derive(Debug, Clone, Serialize, Deserialize, Eq, PartialEq)] diff --git a/rust/cymbal/src/core/types/frames/releases.rs b/rust/cymbal/src/core/types/frames/releases.rs index e64c6c0edea8..9915e17cadc6 100644 --- a/rust/cymbal/src/core/types/frames/releases.rs +++ b/rust/cymbal/src/core/types/frames/releases.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -5,7 +7,11 @@ use sha2::{Digest, Sha512}; use sqlx::Executor; use uuid::Uuid; -#[derive(Debug, Clone, Eq, PartialEq)] +use super::Frame; + +// Serialized only on the internal resolution-service wire (`Done.releases_json`), never into the +// clickhouse-bound event JSON — `Frame.release` stays `#[serde(skip)]`. +#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)] pub struct ReleaseRecord { pub id: Uuid, pub team_id: i32, @@ -71,6 +77,56 @@ impl ReleaseRecord { Ok(row) } + pub async fn for_symbol_set_ref<'c, E>( + e: E, + symbol_set_ref: &str, + team_id: i32, + ) -> Result, sqlx::Error> + where + E: Executor<'c, Database = sqlx::Postgres>, + { + let row = sqlx::query_as!( + Self, + r#" + SELECT r.id, r.team_id, r.hash_id, r.created_at, r.version, r.project, r.metadata + FROM posthog_errortrackingsymbolset ss + INNER JOIN posthog_errortrackingrelease r ON ss.release_id = r.id + WHERE ss.ref = $1 AND ss.team_id = $2 + "#, + symbol_set_ref, + team_id + ) + .fetch_optional(e) + .await?; + + Ok(row) + } + + pub async fn for_symbol_set_id<'c, E>( + e: E, + symbol_set_id: Uuid, + team_id: i32, + ) -> Result, sqlx::Error> + where + E: Executor<'c, Database = sqlx::Postgres>, + { + let row = sqlx::query_as!( + Self, + r#" + SELECT r.id, r.team_id, r.hash_id, r.created_at, r.version, r.project, r.metadata + FROM posthog_errortrackingsymbolset ss + INNER JOIN posthog_errortrackingrelease r ON ss.release_id = r.id + WHERE ss.id = $1 AND ss.team_id = $2 + "#, + symbol_set_id, + team_id + ) + .fetch_optional(e) + .await?; + + Ok(row) + } + pub fn to_info(&self) -> ReleaseInfo { ReleaseInfo { project: self.project.clone(), @@ -80,6 +136,26 @@ impl ReleaseRecord { } } + /// Distinct releases attached to the given frames, deduped by release id, in first-seen order. + pub fn collect_from_frames<'a>(frames: impl Iterator) -> Vec { + let mut seen = HashSet::new(); + let mut out = Vec::new(); + for release in frames.filter_map(|f| f.release.as_ref()) { + if seen.insert(release.id) { + out.push(release.clone()); + } + } + out + } + + /// The most recently created release, with ties broken by id so the pick is deterministic + /// regardless of frame order. + pub fn latest(releases: impl IntoIterator) -> Option { + releases + .into_iter() + .max_by_key(|release| (release.created_at, release.id)) + } + /// Rough in-memory footprint, for the release cache's weigher. `metadata` is a free-form /// JSON column any client can write, so it dominates and is the only reason this exists — /// without it a cache bounded on entry count would be unbounded in bytes. Only has to be diff --git a/rust/cymbal/src/core/types/langs/apple.rs b/rust/cymbal/src/core/types/langs/apple.rs index be1b2a774338..ddd9a776d3a5 100644 --- a/rust/cymbal/src/core/types/langs/apple.rs +++ b/rust/cymbal/src/core/types/langs/apple.rs @@ -224,6 +224,7 @@ impl RawAppleFrame { resolve_failure: None, junk_drawer: None, + release: None, synthetic: self.meta.synthetic, context: None, suspicious: false, @@ -291,6 +292,7 @@ impl RawAppleFrame { resolved: false, resolve_failure: Some(err.to_string()), junk_drawer: None, + release: None, synthetic: self.meta.synthetic, context: None, suspicious: false, @@ -382,6 +384,7 @@ impl From<&RawAppleFrame> for Frame { resolve_failure: None, junk_drawer: None, + release: None, synthetic: raw.meta.synthetic, context: None, suspicious: false, diff --git a/rust/cymbal/src/core/types/langs/custom.rs b/rust/cymbal/src/core/types/langs/custom.rs index 2b91df43225a..c76908cc5eb0 100644 --- a/rust/cymbal/src/core/types/langs/custom.rs +++ b/rust/cymbal/src/core/types/langs/custom.rs @@ -101,6 +101,7 @@ impl From<&CustomFrame> for Frame { junk_drawer: None, context: value.get_context(), + release: None, synthetic: value.meta.synthetic, suspicious: false, module: value.module.clone(), diff --git a/rust/cymbal/src/core/types/langs/dart.rs b/rust/cymbal/src/core/types/langs/dart.rs index b3f308cee908..9bd190e5882d 100644 --- a/rust/cymbal/src/core/types/langs/dart.rs +++ b/rust/cymbal/src/core/types/langs/dart.rs @@ -50,6 +50,7 @@ impl From<&RawDartFrame> for Frame { resolve_failure: None, junk_drawer: None, + release: None, synthetic: raw.meta.synthetic, context: None, suspicious: false, diff --git a/rust/cymbal/src/core/types/langs/go.rs b/rust/cymbal/src/core/types/langs/go.rs index d848f29562a1..1e78055fa0bb 100644 --- a/rust/cymbal/src/core/types/langs/go.rs +++ b/rust/cymbal/src/core/types/langs/go.rs @@ -40,6 +40,7 @@ impl From<&RawGoFrame> for Frame { synthetic: frame.meta.synthetic, junk_drawer: None, context: None, + release: None, suspicious: false, module: None, code_variables: None, diff --git a/rust/cymbal/src/core/types/langs/hermes.rs b/rust/cymbal/src/core/types/langs/hermes.rs index 566b87f4c2e5..2a059a5271e9 100644 --- a/rust/cymbal/src/core/types/langs/hermes.rs +++ b/rust/cymbal/src/core/types/langs/hermes.rs @@ -156,6 +156,7 @@ impl From<(&RawHermesFrame, HermesError)> for Frame { junk_drawer: None, code_variables: None, context: None, + release: None, suspicious: false, module: None, }; @@ -197,6 +198,7 @@ impl From<(&RawHermesFrame, Token<'_>, Option, usize)> for Frame { junk_drawer: None, code_variables: None, context: get_token_context(&token, token.get_src_line() as usize, context_lines), + release: None, suspicious: false, module: None, }; @@ -231,6 +233,7 @@ impl From<&RawHermesFrame> for Frame { junk_drawer: None, code_variables: None, context: None, + release: None, synthetic: raw_frame.meta.synthetic, suspicious: false, module: None, diff --git a/rust/cymbal/src/core/types/langs/java.rs b/rust/cymbal/src/core/types/langs/java.rs index 4bda68721d40..0edee149c145 100644 --- a/rust/cymbal/src/core/types/langs/java.rs +++ b/rust/cymbal/src/core/types/langs/java.rs @@ -191,6 +191,7 @@ impl<'a> From<(&'a RawJavaFrame, StackFrame<'a>)> for Frame { junk_drawer: None, code_variables: None, + release: None, synthetic: raw.meta.synthetic, context: None, suspicious: false, @@ -222,6 +223,7 @@ impl From<(&RawJavaFrame, ProguardError)> for Frame { resolve_failure, junk_drawer: None, code_variables: None, + release: None, synthetic: raw.meta.synthetic, context: None, suspicious: false, diff --git a/rust/cymbal/src/core/types/langs/js.rs b/rust/cymbal/src/core/types/langs/js.rs index ee89dd9a85d2..bcfa57a9d189 100644 --- a/rust/cymbal/src/core/types/langs/js.rs +++ b/rust/cymbal/src/core/types/langs/js.rs @@ -235,6 +235,7 @@ impl From<(&RawJSFrame, SourceLocation<'_>, usize)> for Frame { junk_drawer: None, code_variables: None, context: get_sourcelocation_context(&token, context_lines), + release: None, synthetic: raw_frame.meta.synthetic, suspicious, module: None, @@ -293,6 +294,7 @@ impl From<(&RawJSFrame, JsResolveErr, &FrameLocation)> for Frame { junk_drawer: None, code_variables: None, context: None, + release: None, synthetic: raw_frame.meta.synthetic, suspicious: false, module: None, @@ -332,6 +334,7 @@ impl From<&RawJSFrame> for Frame { junk_drawer: None, code_variables: None, context: None, + release: None, synthetic: raw_frame.meta.synthetic, suspicious: false, module: None, diff --git a/rust/cymbal/src/core/types/langs/native.rs b/rust/cymbal/src/core/types/langs/native.rs index 8f71c70c7c14..7b060082a0f5 100644 --- a/rust/cymbal/src/core/types/langs/native.rs +++ b/rust/cymbal/src/core/types/langs/native.rs @@ -354,6 +354,7 @@ impl RawNativeFrame { resolve_failure: None, junk_drawer: None, + release: None, synthetic: self.meta.synthetic, context: None, suspicious: false, @@ -403,6 +404,7 @@ impl RawNativeFrame { resolved: false, resolve_failure: Some(err.to_string()), junk_drawer: None, + release: None, synthetic: self.meta.synthetic, context: None, suspicious: false, @@ -565,6 +567,7 @@ impl From<&RawNativeFrame> for Frame { resolve_failure: None, junk_drawer: None, + release: None, synthetic: raw.meta.synthetic, context: None, suspicious: false, diff --git a/rust/cymbal/src/core/types/langs/node.rs b/rust/cymbal/src/core/types/langs/node.rs index bb3b2a4a866f..6c27f912a387 100644 --- a/rust/cymbal/src/core/types/langs/node.rs +++ b/rust/cymbal/src/core/types/langs/node.rs @@ -189,6 +189,7 @@ impl From<&RawNodeFrame> for Frame { junk_drawer: None, context: raw.get_context(), + release: None, synthetic: raw.meta.synthetic, suspicious: false, module: raw.module.clone(), @@ -234,6 +235,7 @@ impl From<(&RawNodeFrame, SourceLocation<'_>, usize)> for Frame { junk_drawer: None, code_variables: None, context: get_sourcelocation_context(&location, context_lines), + release: None, synthetic: raw_frame.meta.synthetic, suspicious: false, module: raw_frame.module.clone(), @@ -291,6 +293,7 @@ impl From<(&RawNodeFrame, JsResolveErr)> for Frame { junk_drawer: None, code_variables: None, context: raw_frame.get_context(), + release: None, synthetic: raw_frame.meta.synthetic, suspicious: false, module: raw_frame.module.clone(), diff --git a/rust/cymbal/src/core/types/langs/php.rs b/rust/cymbal/src/core/types/langs/php.rs index 2a52e2fc3083..497d7969993b 100644 --- a/rust/cymbal/src/core/types/langs/php.rs +++ b/rust/cymbal/src/core/types/langs/php.rs @@ -94,6 +94,7 @@ impl From<&RawPHPFrame> for Frame { junk_drawer: None, context: raw.get_context(), + release: None, synthetic: raw.meta.synthetic, suspicious: false, module: None, diff --git a/rust/cymbal/src/core/types/langs/python.rs b/rust/cymbal/src/core/types/langs/python.rs index 45885b29a2d4..33ae57d09d5f 100644 --- a/rust/cymbal/src/core/types/langs/python.rs +++ b/rust/cymbal/src/core/types/langs/python.rs @@ -347,6 +347,7 @@ impl From<&RawPythonFrame> for Frame { junk_drawer: None, context: raw.get_context(), + release: None, synthetic: raw.meta.synthetic, suspicious: false, module: raw.module.clone(), diff --git a/rust/cymbal/src/core/types/langs/ruby.rs b/rust/cymbal/src/core/types/langs/ruby.rs index 7f2464ccfcb9..b41ba51ae7c7 100644 --- a/rust/cymbal/src/core/types/langs/ruby.rs +++ b/rust/cymbal/src/core/types/langs/ruby.rs @@ -90,6 +90,7 @@ impl From<&RawRubyFrame> for Frame { junk_drawer: None, context: raw.get_context(), + release: None, synthetic: raw.meta.synthetic, suspicious: false, module: None, diff --git a/rust/cymbal/src/modes/processing/fingerprinting/mod.rs b/rust/cymbal/src/modes/processing/fingerprinting/mod.rs index fc66aac7d9f2..900bd0639474 100644 --- a/rust/cymbal/src/modes/processing/fingerprinting/mod.rs +++ b/rust/cymbal/src/modes/processing/fingerprinting/mod.rs @@ -482,6 +482,7 @@ mod test { junk_drawer: None, code_variables: None, context: None, + release: None, synthetic: false, suspicious: false, module: None, diff --git a/rust/cymbal/src/modes/processing/normalization.rs b/rust/cymbal/src/modes/processing/normalization.rs index 90190429a64c..f976d717cc76 100644 --- a/rust/cymbal/src/modes/processing/normalization.rs +++ b/rust/cymbal/src/modes/processing/normalization.rs @@ -661,6 +661,7 @@ mod test { junk_drawer: None, code_variables: None, context: None, + release: None, module: None, }; diff --git a/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs b/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs index ff9ac7c89ed2..558246d9fb18 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs @@ -427,6 +427,7 @@ mod tests { id: item.id, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: item.exception_json, + releases_json: Vec::new(), })), }; tx.send(Ok(outcome)).await.expect("test receiver is alive"); diff --git a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs index 3ee12ce127f9..2e77f89ac39f 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs @@ -8,6 +8,7 @@ use tokio::sync::Semaphore; use crate::{ error::UnhandledError, + frames::releases::ReleaseRecord, stages::{ pipeline::ParsedPipelineItem, resolution::{ @@ -166,6 +167,9 @@ struct ResolvedRemoteItem { event_slot: usize, exception_slot: usize, exception: Exception, + /// Releases bound to the symbol sets that resolved this exception's frames, from the + /// response's `releases_json` sidecar — `Frame.release` itself does not survive the wire. + releases: Vec, } // ───────────────────────────────────────────────────────────────────────────── @@ -233,6 +237,7 @@ async fn resolve_remote_events( item.event_slot, item.exception_slot ))); } + event_slot.evt.add_frame_releases(item.releases); } event_slots diff --git a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs index efbdc5475c62..0e83c48d8750 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs @@ -17,6 +17,7 @@ use cymbal_proto::cymbal::resolution::v1::{resolve_outcome, ErrorKind, ResolveOu use tonic::Status; use crate::error::UnhandledError; +use crate::frames::releases::ReleaseRecord; use crate::metric_consts::{ REMOTE_RESOLUTION_ERROR_KINDS, REMOTE_RESOLUTION_LATENCY, REMOTE_RESOLUTION_OVERLOAD_ESCALATIONS, REMOTE_RESOLUTION_REQUESTS, @@ -151,13 +152,17 @@ pub(super) async fn resolve_work_item( }; match decision { - ItemDecision::Done(exception) => { + ItemDecision::Done { + exception, + releases, + } => { metrics::counter!(REMOTE_RESOLUTION_REQUESTS, "outcome" => "ok").increment(1); record_reroute_depth("ok", attempts_used); return Ok(ResolvedRemoteItem { event_slot: work_item.event_slot, exception_slot: work_item.exception_slot, exception, + releases, }); } ItemDecision::Overloaded(message) => { @@ -275,7 +280,10 @@ fn single_outcome( #[derive(Debug)] enum ItemDecision { - Done(Exception), + Done { + exception: Exception, + releases: Vec, + }, Overloaded(String), Retry { message: String, @@ -303,7 +311,24 @@ fn classify_outcome( format!("invalid_done_payload: failed to parse resolved exception: {err}"), ) })?; - Ok(ItemDecision::Done(exception)) + // Empty bytes means an older server that predates the field, or no frame resolved + // with a release — both are simply "no releases". + let releases = if done.releases_json.is_empty() { + Vec::new() + } else { + serde_json::from_slice::>(&done.releases_json).map_err( + |err| { + terminal_item_error( + work_item.token, + format!("invalid_done_payload: failed to parse frame releases: {err}"), + ) + }, + )? + }; + Ok(ItemDecision::Done { + exception, + releases, + }) } resolve_outcome::Result::Retry(retry) => { let retry_after = (retry.retry_after_ms > 0) @@ -430,16 +455,66 @@ mod tests { #[test] fn classify_outcome_parses_done_exception() { let work_item = work_item(7); + // Empty `releases_json` is what a server predating the field sends; it must decode as + // "no releases", not an error. let outcome = ResolveOutcome { id: 7, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: serde_json::to_vec(&exception("Resolved")) .expect("valid exception"), + releases_json: Vec::new(), })), }; let decision = classify_outcome(&work_item, outcome).expect("done outcome"); - assert!(matches!(decision, ItemDecision::Done(exc) if exc.exception_type == "Resolved")); + assert!(matches!( + decision, + ItemDecision::Done { exception, releases } + if exception.exception_type == "Resolved" && releases.is_empty() + )); + } + + #[test] + fn classify_outcome_parses_done_releases_sidecar() { + let release = ReleaseRecord { + id: uuid::Uuid::now_v7(), + team_id: 42, + hash_id: "hash".to_string(), + created_at: chrono::Utc::now(), + version: "1.2.3".to_string(), + project: "my-app".to_string(), + metadata: None, + }; + let work_item = work_item(7); + let outcome = ResolveOutcome { + id: 7, + result: Some(resolve_outcome::Result::Done(Done { + resolved_exception_json: serde_json::to_vec(&exception("Resolved")) + .expect("valid exception"), + releases_json: serde_json::to_vec(&vec![release.clone()]).expect("valid releases"), + })), + }; + + let decision = classify_outcome(&work_item, outcome).expect("done outcome"); + assert!(matches!( + decision, + ItemDecision::Done { releases, .. } if releases == vec![release] + )); + } + + #[test] + fn classify_outcome_rejects_malformed_releases_sidecar() { + let work_item = work_item(7); + let outcome = ResolveOutcome { + id: 7, + result: Some(resolve_outcome::Result::Done(Done { + resolved_exception_json: serde_json::to_vec(&exception("Resolved")) + .expect("valid exception"), + releases_json: b"not json".to_vec(), + })), + }; + + assert!(classify_outcome(&work_item, outcome).is_err()); } #[test] diff --git a/rust/cymbal/src/modes/processing/types/exception_event.rs b/rust/cymbal/src/modes/processing/types/exception_event.rs index db31b2239e67..49868a51c50e 100644 --- a/rust/cymbal/src/modes/processing/types/exception_event.rs +++ b/rust/cymbal/src/modes/processing/types/exception_event.rs @@ -29,6 +29,11 @@ pub struct Parsed { /// The release resolved from the event's `$release_id` or mobile app metadata, if any. Set by /// `EventReleaseResolver` and emitted as `$exception_release` at `into_resolved`. pub(crate) event_release: Option, + /// Releases bound to the symbol sets that resolved this event's frames, accumulated from the + /// remote resolution response sidecar. The local resolution path instead leaves them on + /// `Frame.release`; both sources are merged at `into_resolved`, where the latest one becomes + /// the `$exception_release` fallback when `event_release` is unset. + pub(crate) frame_releases: Vec, } #[derive(Debug, Clone)] @@ -47,14 +52,27 @@ impl ResolvedMetadata { fn from_exception_list( exception_list: &ExceptionList, event_release: Option<&ReleaseRecord>, + frame_releases: Vec, ) -> Self { + // The event-level release (`$release_id` / mobile app-metadata hash) is authoritative; + // frame-derived releases only fill in when it resolved nothing, picking the latest so an + // event whose stack mixes chunks from several releases reports the newest one. + let release = event_release + .cloned() + .or_else(|| { + let mut candidates = frame_releases; + candidates.extend(exception_list.get_frame_releases()); + ReleaseRecord::latest(candidates) + }) + .map(|release| release.to_info()); + Self { sources: exception_list.get_unique_sources(), types: exception_list.get_unique_types(), messages: exception_list.get_unique_messages(), functions: exception_list.get_unique_functions(), handled: exception_list.get_is_handled(), - release: event_release.map(ReleaseRecord::to_info), + release, } } } @@ -217,10 +235,15 @@ impl ExceptionEvent { self.state.event_release = release; } + pub(crate) fn add_frame_releases(&mut self, releases: Vec) { + self.state.frame_releases.extend(releases); + } + pub(crate) fn into_resolved(self) -> ExceptionEvent { let metadata = ResolvedMetadata::from_exception_list( &self.exception_list, self.state.event_release.as_ref(), + self.state.frame_releases.clone(), ); self.map_state(|state| Resolved { metadata, @@ -583,6 +606,7 @@ impl TryFrom for ExceptionEvent { legacy_order_exception_list, legacy_order_resolved: None, event_release: None, + frame_releases: Vec::new(), }, }) } @@ -713,35 +737,124 @@ mod tests { } fn release_record(hash_id: &str) -> ReleaseRecord { + release_record_at(hash_id, 0) + } + + fn release_record_at(hash_id: &str, created_secs: i64) -> ReleaseRecord { ReleaseRecord { id: Uuid::now_v7(), team_id: 42, hash_id: hash_id.to_string(), - created_at: chrono::DateTime::from_timestamp(0, 0).unwrap(), - version: "1.2.3".to_string(), + created_at: chrono::DateTime::from_timestamp(created_secs, 0).unwrap(), + version: format!("1.2.{created_secs}"), project: "my-app".to_string(), metadata: None, } } + fn frame_with_release(release: Option) -> crate::frames::Frame { + crate::frames::Frame { + frame_id: common_types::error_tracking::FrameId::placeholder(), + mangled_name: "f".to_string(), + line: None, + column: None, + source: None, + module: None, + in_app: true, + resolved_name: None, + lang: "javascript".to_string(), + resolved: true, + resolve_failure: None, + synthetic: false, + suspicious: false, + junk_drawer: None, + code_variables: None, + context: None, + release, + } + } + + fn exception_list_with_frames(frames: Vec) -> ExceptionList { + ExceptionList(vec![crate::types::Exception { + exception_id: None, + exception_type: "Error".to_string(), + exception_message: "boom".to_string(), + mechanism: None, + module: None, + thread_id: None, + stack: Some(crate::types::Stacktrace::Resolved { frames }), + }]) + } + #[test] fn event_release_populates_the_singular_release() { - // The event-level release (from `$release_id` or mobile app metadata) is the sole source of - // `$exception_release`; it does not depend on any frame carrying a release. let metadata = ResolvedMetadata::from_exception_list( &ExceptionList::default(), Some(&release_record("hash-abc")), + Vec::new(), ); assert!(metadata.release.is_some()); } #[test] fn missing_event_release_leaves_the_release_unset() { - // Without an event-level release there is nothing to emit; there is no per-frame fallback. - let metadata = ResolvedMetadata::from_exception_list(&ExceptionList::default(), None); + // Without an event-level release and without any frame-derived candidate there is nothing + // to emit. + let metadata = + ResolvedMetadata::from_exception_list(&ExceptionList::default(), None, Vec::new()); assert!(metadata.release.is_none()); } + #[test] + fn event_release_takes_precedence_over_frame_releases() { + // `$release_id`/app-hash resolution is authoritative even when frame-derived releases are + // newer; the fallback only fills a gap, it never overrides. + let event_release = release_record_at("event-hash", 100); + let newer_frame_release = release_record_at("frame-hash", 5_000); + + let metadata = ResolvedMetadata::from_exception_list( + &ExceptionList::default(), + Some(&event_release), + vec![newer_frame_release], + ); + + let expected = serde_json::to_value(event_release.to_info()).unwrap(); + assert_eq!(serde_json::to_value(&metadata.release).unwrap(), expected); + } + + #[test] + fn frame_release_fallback_picks_the_latest_across_both_sources() { + // Without an event-level release, the fallback considers the remote sidecar and the + // releases local symbolication left on the frames, and picks the most recently created. + let sidecar_release = release_record_at("sidecar-hash", 100); + let latest_frame_release = release_record_at("frame-hash", 5_000); + let exception_list = exception_list_with_frames(vec![frame_with_release(Some( + latest_frame_release.clone(), + ))]); + + let metadata = ResolvedMetadata::from_exception_list( + &exception_list, + None, + vec![sidecar_release.clone()], + ); + + let expected = serde_json::to_value(latest_frame_release.to_info()).unwrap(); + assert_eq!(serde_json::to_value(&metadata.release).unwrap(), expected); + + // And the other way around: a newer sidecar release beats an older frame-attached one. + let exception_list = + exception_list_with_frames(vec![frame_with_release(Some(sidecar_release))]); + let newer_sidecar = release_record_at("sidecar-hash-2", 9_000); + let metadata = ResolvedMetadata::from_exception_list( + &exception_list, + None, + vec![newer_sidecar.clone()], + ); + + let expected = serde_json::to_value(newer_sidecar.to_info()).unwrap(); + assert_eq!(serde_json::to_value(&metadata.release).unwrap(), expected); + } + #[test] fn exception_release_emitted_only_when_a_release_resolves() { let issue = Issue { diff --git a/rust/cymbal/src/modes/processing/types/mod.rs b/rust/cymbal/src/modes/processing/types/mod.rs index 05a4caca9fe0..d5dbeb7b0a55 100644 --- a/rust/cymbal/src/modes/processing/types/mod.rs +++ b/rust/cymbal/src/modes/processing/types/mod.rs @@ -7,7 +7,7 @@ use std::ops::{Deref, DerefMut}; use uuid::Uuid; use crate::fingerprinting::{FingerprintRecordPart, FingerprintVersion}; -use crate::frames::releases::ReleaseInfo; +use crate::frames::releases::{ReleaseInfo, ReleaseRecord}; use crate::frames::{Frame, RawFrame}; use crate::langs::native::DebugImage; use crate::metric_consts::POSTHOG_SDK_EXCEPTION_RESOLVED; @@ -93,6 +93,13 @@ impl ExceptionList { .and_then(|m| m.handled) .unwrap_or(false) } + + /// Releases attached in-memory to this list's frames by local symbolication. Frames that came + /// back from the remote resolution service never carry one (`Frame.release` is not + /// serialized); their releases arrive via the response sidecar instead. + pub fn get_frame_releases(&self) -> Vec { + ReleaseRecord::collect_from_frames(self.get_frames_iter()) + } } /// Untrusted exception properties accepted from ClickHouse and SDK event payloads. diff --git a/rust/cymbal/src/modes/resolution/README.md b/rust/cymbal/src/modes/resolution/README.md index 588fed4fd80f..83f5247fc5e9 100644 --- a/rust/cymbal/src/modes/resolution/README.md +++ b/rust/cymbal/src/modes/resolution/README.md @@ -46,7 +46,7 @@ events ──HTTP─────▶│ cymbal The contract is intentionally split across two streams: -- **`Resolve`** is bidirectional work traffic. The caller sends independent `ResolveItem`s, each with a per-stream id, `team_id`, serialized exception JSON, JSON `metadata` bytes, and an item deadline. The server emits an `Accepted` outcome when it admits an item, then exactly one terminal `ResolveOutcome` with the same id: `Done`, `Retry`, or `Error`. +- **`Resolve`** is bidirectional work traffic. The caller sends independent `ResolveItem`s, each with a per-stream id, `team_id`, serialized exception JSON, JSON `metadata` bytes, and an item deadline. The server emits an `Accepted` outcome when it admits an item, then exactly one terminal `ResolveOutcome` with the same id: `Done`, `Retry`, or `Error`. A `Done` carries the resolved exception JSON plus a `releases_json` sidecar: the releases bound to the symbol sets that resolved the exception's frames, deduped by release id. `Frame.release` is never serialized, so the sidecar is the only way releases cross this boundary; callers must treat empty bytes as "no releases" (older servers do not set the field). - **`Subscribe`** is endpoint freshness, draining, and soft load state. The cymbal-side `EndpointPool` opens one long-lived stream per pod and treats the latest `LoadEvent` as a freshness snapshot plus an `in_flight` / `max_in_flight` routing bias. `LoadEvent` does not carry overload state or suggested batch sizing. `Error.kind` is the shared control-flow surface: diff --git a/rust/cymbal/src/modes/resolution/service.rs b/rust/cymbal/src/modes/resolution/service.rs index bfc36ec12c2e..4bfe184f6bdd 100644 --- a/rust/cymbal/src/modes/resolution/service.rs +++ b/rust/cymbal/src/modes/resolution/service.rs @@ -66,8 +66,10 @@ impl CymbalResolutionService { ResolutionStage { symbol_resolver: self.symbol_resolver.clone(), symbol_resolution_limiter: self.symbol_resolution_limiter.clone(), - // The resolution server only symbolicates frames; event-level release resolution runs - // on the processing side, so no release pool or cache is needed here. + // Event-level release resolution (`$release_id` / app-metadata hash) runs on the + // processing side, so no release pool or cache is needed here. The frame-derived + // releases this server returns in the `Done` sidecar come from the symbol-set join + // inside the symbol resolver, which uses its own pool. posthog_pool: None, release_cache: ReleaseCache::disabled(), // The cymbal-resolution server never enables remote mode itself; diff --git a/rust/cymbal/src/modes/resolution/service/resolve.rs b/rust/cymbal/src/modes/resolution/service/resolve.rs index 5f0aecfe37f3..9f1225afd420 100644 --- a/rust/cymbal/src/modes/resolution/service/resolve.rs +++ b/rust/cymbal/src/modes/resolution/service/resolve.rs @@ -8,11 +8,12 @@ use tonic::{Status, Streaming}; use tracing::{debug, warn}; use crate::error::UnhandledError; +use crate::frames::releases::ReleaseRecord; use crate::langs::native::DebugImage; use crate::stages::resolution::exception::ExceptionResolver; use crate::stages::resolution::frame::FrameResolver; use crate::stages::resolution::ResolutionStage; -use crate::types::Exception; +use crate::types::{Exception, Stacktrace}; use cymbal_proto::cymbal::resolution::v1::{ resolve_outcome, Accepted, Done, Error as ItemError, ResolveItem, ResolveOutcome, }; @@ -152,7 +153,8 @@ async fn process_item( match tokio::time::timeout(deadline, resolve_item(&stage, &item)).await { Ok(Ok(resolved)) => ( resolve_outcome::Result::Done(Done { - resolved_exception_json: resolved, + resolved_exception_json: resolved.exception_json, + releases_json: resolved.releases_json, }), "done", "ok", @@ -230,7 +232,15 @@ enum ItemFailure { Unhandled(String), } -async fn resolve_item(stage: &ResolutionStage, item: &ResolveItem) -> Result, ItemFailure> { +struct ResolvedItemPayload { + exception_json: Vec, + releases_json: Vec, +} + +async fn resolve_item( + stage: &ResolutionStage, + item: &ResolveItem, +) -> Result { let exception: Exception = serde_json::from_slice(&item.exception_json) .map_err(|e| ItemFailure::InvalidPayload(format!("invalid exception_json: {e}")))?; @@ -245,8 +255,31 @@ async fn resolve_item(stage: &ResolutionStage, item: &ResolveItem) -> Result ItemFailure::Unhandled(err), })?; - serde_json::to_vec(&resolved) - .map_err(|e| ItemFailure::Unhandled(format!("serialize resolved exception: {e}"))) + let releases_json = frame_releases_json(&resolved)?; + let exception_json = serde_json::to_vec(&resolved) + .map_err(|e| ItemFailure::Unhandled(format!("serialize resolved exception: {e}")))?; + + Ok(ResolvedItemPayload { + exception_json, + releases_json, + }) +} + +// `Frame.release` is `#[serde(skip)]`, so the releases the symbol-set join attached during +// resolution would be lost in `resolved_exception_json`; they cross the wire in this sidecar +// instead. Empty bytes (not an empty JSON array) when nothing resolved, matching what an older +// server sends. +fn frame_releases_json(exception: &Exception) -> Result, ItemFailure> { + let frames = match &exception.stack { + Some(Stacktrace::Resolved { frames }) => frames.as_slice(), + _ => &[], + }; + let releases = ReleaseRecord::collect_from_frames(frames.iter()); + if releases.is_empty() { + return Ok(Vec::new()); + } + serde_json::to_vec(&releases) + .map_err(|e| ItemFailure::Unhandled(format!("serialize frame releases: {e}"))) } fn debug_images_from_metadata(metadata: &[u8]) -> Result, ItemFailure> { diff --git a/rust/cymbal/tests/common/mod.rs b/rust/cymbal/tests/common/mod.rs index 4d55ebf8c099..670143c3aa08 100644 --- a/rust/cymbal/tests/common/mod.rs +++ b/rust/cymbal/tests/common/mod.rs @@ -259,6 +259,7 @@ fn done_outcome(item: &ResolveItem) -> ResolveOutcome { id: item.id, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: item.exception_json.clone(), + releases_json: Vec::new(), })), } } @@ -488,6 +489,16 @@ pub fn build_event_with( team_id: i32, uuid: Uuid, debug_images: Vec, +) -> ExceptionEvent { + build_event_with_raw_frames(num_exceptions, team_id, uuid, debug_images, Vec::new()) +} + +pub fn build_event_with_raw_frames( + num_exceptions: usize, + team_id: i32, + uuid: Uuid, + debug_images: Vec, + frames: Vec, ) -> ExceptionEvent { let exceptions: Vec = (0..num_exceptions) .map(|i| Exception { @@ -497,7 +508,9 @@ pub fn build_event_with( mechanism: None, module: None, thread_id: None, - stack: Some(Stacktrace::Raw { frames: vec![] }), + stack: Some(Stacktrace::Raw { + frames: frames.clone(), + }), }) .collect(); AnyEvent { diff --git a/rust/cymbal/tests/event.rs b/rust/cymbal/tests/event.rs index 92da4d14257d..e1f95a6250c1 100644 --- a/rust/cymbal/tests/event.rs +++ b/rust/cymbal/tests/event.rs @@ -103,6 +103,7 @@ fn make_frame_js(name: &str) -> Frame { junk_drawer: None, code_variables: None, context: None, + release: None, } } @@ -125,6 +126,7 @@ fn make_frame_ts(name: &str) -> Frame { junk_drawer: None, code_variables: None, context: None, + release: None, } } diff --git a/rust/cymbal/tests/fingerprint_golden.rs b/rust/cymbal/tests/fingerprint_golden.rs index de0f33a798e6..8ce6bb2504b7 100644 --- a/rust/cymbal/tests/fingerprint_golden.rs +++ b/rust/cymbal/tests/fingerprint_golden.rs @@ -36,6 +36,7 @@ fn frame( junk_drawer: None, code_variables: None, context: None, + release: None, synthetic: false, suspicious: false, module: module.map(String::from), diff --git a/rust/cymbal/tests/remote_resolution_parity.rs b/rust/cymbal/tests/remote_resolution_parity.rs index d6afa24b6704..58699dd4f099 100644 --- a/rust/cymbal/tests/remote_resolution_parity.rs +++ b/rust/cymbal/tests/remote_resolution_parity.rs @@ -19,8 +19,9 @@ use std::sync::Arc; use std::time::Duration; use async_trait::async_trait; -use common::{build_event, make_ctx}; +use common::{build_event, build_event_with_raw_frames, make_ctx}; use cymbal::error::{ResolveError, UnhandledError}; +use cymbal::frames::releases::ReleaseRecord; use cymbal::frames::{Frame, RawFrame}; use cymbal::langs::native::DebugImage; use cymbal::modes::resolution::load_monitor::LoadMonitor; @@ -174,3 +175,120 @@ async fn local_and_remote_stages_produce_identical_exception_list_for_empty_stac ); assert_eq!(local_out.metadata().handled, remote_out.metadata().handled); } + +/// Resolver that attaches a fixed release to every frame it resolves, standing in for the +/// symbol-set→release join the real resolver performs. Deterministic like `FakeResolver`. +struct ReleaseAttachingResolver { + release: ReleaseRecord, +} + +#[async_trait] +impl SymbolResolver for ReleaseAttachingResolver { + async fn resolve_raw_frame( + &self, + team_id: TeamId, + frame: &RawFrame, + debug_images: &[DebugImage], + ) -> Result, UnhandledError> { + Ok(vec![Frame { + frame_id: frame.frame_id(team_id, 0, debug_images), + mangled_name: "f".to_string(), + line: Some(42), + column: Some(7), + source: Some("src/app.ts".to_string()), + module: None, + in_app: true, + resolved_name: Some("renderCheckout".to_string()), + lang: "javascript".to_string(), + resolved: true, + resolve_failure: None, + synthetic: false, + suspicious: false, + junk_drawer: None, + code_variables: None, + context: None, + release: Some(self.release.clone()), + }]) + } + + async fn resolve_java_class( + &self, + _team_id: TeamId, + _symbolset_ref: OrChunkId, + _class: String, + ) -> Result { + unreachable!("parity fixtures do not exercise Java class resolution") + } + + async fn resolve_dart_minified_name( + &self, + _team_id: TeamId, + _symbolset_ref: String, + _minified_name: &str, + ) -> Result { + unreachable!("parity fixtures do not exercise Dart name resolution") + } +} + +fn fixed_release() -> ReleaseRecord { + ReleaseRecord { + id: uuid::Uuid::from_u128(7), + team_id: 7, + hash_id: "parity-release-hash".to_string(), + created_at: chrono::DateTime::from_timestamp(1_700_000_000, 0).unwrap(), + version: "9.9.9".to_string(), + project: "parity-app".to_string(), + metadata: None, + } +} + +fn sample_raw_frame() -> RawFrame { + serde_json::from_value(serde_json::json!({ + "platform": "web:javascript", + "filename": "a.js", + "function": "f", + "in_app": true, + "lineno": 1, + "colno": 1, + })) + .expect("valid raw frame") +} + +/// The frame-derived release reaches `$exception_release` identically on both paths, even though +/// it travels differently: in-memory on `Frame.release` locally, via the `releases_json` sidecar +/// remotely. The byte-for-byte exception-list comparison cannot see it (`Frame.release` is +/// serde-skipped), so it gets its own parity assertion. +#[tokio::test] +async fn local_and_remote_stages_produce_identical_exception_release() { + let resolver: Arc = Arc::new(ReleaseAttachingResolver { + release: fixed_release(), + }); + let addr = spawn_cymbal_resolution_with_resolver(resolver.clone()).await; + let ctx = make_ctx(&[addr], 0, Duration::from_secs(5)).await; + + let evt = build_event_with_raw_frames( + 2, + 7, + uuid::Uuid::now_v7(), + Vec::new(), + vec![sample_raw_frame()], + ); + let local_out = run_stage(local_stage(resolver.clone()), evt.clone()).await; + let remote_out = run_stage(remote_stage(resolver, ctx), evt).await; + + let local_json = serde_json::to_value(local_out.exception_list()).unwrap(); + let remote_json = serde_json::to_value(remote_out.exception_list()).unwrap(); + assert_eq!(local_json, remote_json, "exception_list parity"); + + let expected = serde_json::to_value(fixed_release().to_info()).unwrap(); + assert_eq!( + serde_json::to_value(&local_out.metadata().release).unwrap(), + expected, + "local path resolves the frame-derived release" + ); + assert_eq!( + serde_json::to_value(&remote_out.metadata().release).unwrap(), + expected, + "remote path resolves the frame-derived release" + ); +} diff --git a/rust/cymbal/tests/resolution_service_tests.rs b/rust/cymbal/tests/resolution_service_tests.rs index c3c8f00c7074..14b99f59b49c 100644 --- a/rust/cymbal/tests/resolution_service_tests.rs +++ b/rust/cymbal/tests/resolution_service_tests.rs @@ -6,6 +6,7 @@ use std::time::Duration; use async_trait::async_trait; use cymbal::error::{ResolveError, UnhandledError}; +use cymbal::frames::releases::ReleaseRecord; use cymbal::frames::{Frame, RawFrame}; use cymbal::langs::native::DebugImage; use cymbal::modes::resolution::load_monitor::LoadMonitor; @@ -342,6 +343,77 @@ async fn raw_frames_are_resolved_into_done_payload() { assert_eq!(frames, vec![expected_wire_frame]); } +#[tokio::test] +async fn frame_releases_are_emitted_in_the_done_sidecar_and_kept_out_of_the_exception() { + let release = ReleaseRecord { + id: uuid::Uuid::from_u128(7), + team_id: 123, + hash_id: "sidecar-hash".to_string(), + created_at: chrono::DateTime::from_timestamp(1_700_000_000, 0).unwrap(), + version: "9.9.9".to_string(), + project: "my-app".to_string(), + metadata: None, + }; + let raw_frame = sample_raw_frame(); + // Two resolved frames carrying the same release: the sidecar must dedupe by release id. + let mut first = sample_resolved_frame(&raw_frame); + first.frame_id = raw_frame.frame_id(123, 0, &[]); + first.release = Some(release.clone()); + let mut second = sample_resolved_frame(&raw_frame); + second.frame_id = raw_frame.frame_id(123, 1, &[]); + second.release = Some(release.clone()); + let service = make_service(FakeResolver { + fail_unhandled: false, + resolved_frames: vec![first, second], + }); + let mut exc = raw_exception("RuntimeError"); + exc.stack = Some(Stacktrace::Raw { + frames: vec![raw_frame], + }); + + let outcomes = resolve_items(service, vec![make_item(1, &exc)]).await; + assert_eq!(outcomes.len(), 1); + let resolve_outcome::Result::Done(done) = outcome_result(&outcomes[0]) else { + panic!("expected Done outcome, got {:?}", outcomes[0]); + }; + + let releases: Vec = + serde_json::from_slice(&done.releases_json).expect("valid releases sidecar"); + assert_eq!(releases, vec![release]); + + // `Frame.release` must stay out of the serialized exception: the sidecar is the only place + // releases cross the wire, and the frame JSON shape doubles as clickhouse output. + let resolved: serde_json::Value = + serde_json::from_slice(&done.resolved_exception_json).expect("valid resolved exception"); + let frames = resolved["stacktrace"]["frames"] + .as_array() + .expect("resolved frames present"); + assert!(frames.iter().all(|frame| frame.get("release").is_none())); +} + +#[tokio::test] +async fn done_sidecar_is_empty_bytes_when_no_frame_has_a_release() { + let raw_frame = sample_raw_frame(); + let mut resolver_frame = sample_resolved_frame(&raw_frame); + resolver_frame.frame_id = raw_frame.frame_id(123, 99, &[]); + let service = make_service(FakeResolver { + fail_unhandled: false, + resolved_frames: vec![resolver_frame], + }); + let mut exc = raw_exception("RuntimeError"); + exc.stack = Some(Stacktrace::Raw { + frames: vec![raw_frame], + }); + + let outcomes = resolve_items(service, vec![make_item(1, &exc)]).await; + let resolve_outcome::Result::Done(done) = outcome_result(&outcomes[0]) else { + panic!("expected Done outcome, got {:?}", outcomes[0]); + }; + + // Empty bytes, not `[]`: byte-identical to what a server predating the field sends. + assert!(done.releases_json.is_empty()); +} + #[tokio::test] async fn bidi_resolve_stream_emits_outcomes_as_items_complete_out_of_order() { let active = Arc::new(AtomicUsize::new(0)); @@ -537,6 +609,7 @@ fn sample_resolved_frame(raw_frame: &RawFrame) -> Frame { junk_drawer: None, code_variables: None, context: None, + release: None, } } From ce18eb9b8cc9c4aadfb0f38d294a366c7432e170 Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Mon, 3 Aug 2026 12:06:31 +0200 Subject: [PATCH 3/9] refactor(error-tracking): carry frame releases inside frame wire json Replace the Done.releases_json sidecar with a serializable Frame.release, so the per-frame release association survives the resolution wire. Strip releases at into_resolved and in the PG frame cache save path so they never reach clickhouse-bound JSON. Also truncate the symbol set ref in the release join to match how stored refs are written. Co-Authored-By: Claude Fable 5 --- proto/cymbal/resolution/v1/resolution.proto | 9 +- rust/cymbal-proto/tests/contract.rs | 22 +--- rust/cymbal/docs/compatibility.md | 2 +- .../src/core/symbolication/symbol/records.rs | 8 +- .../core/symbolication/symbol_store/saving.rs | 2 +- rust/cymbal/src/core/types/frames/mod.rs | 9 +- rust/cymbal/src/core/types/frames/releases.rs | 9 +- .../stages/resolution/remote/mux.rs | 1 - .../stages/resolution/remote/resolver.rs | 5 - .../resolution/remote/resolver/retry.rs | 99 ++++++++--------- .../modes/processing/types/exception_event.rs | 104 +++++++++--------- rust/cymbal/src/modes/processing/types/mod.rs | 17 ++- rust/cymbal/src/modes/resolution/README.md | 2 +- rust/cymbal/src/modes/resolution/service.rs | 2 +- .../src/modes/resolution/service/resolve.rs | 47 ++------ rust/cymbal/tests/common/mod.rs | 1 - rust/cymbal/tests/remote_resolution_parity.rs | 8 +- rust/cymbal/tests/resolution_service_tests.rs | 51 +++------ 18 files changed, 167 insertions(+), 231 deletions(-) diff --git a/proto/cymbal/resolution/v1/resolution.proto b/proto/cymbal/resolution/v1/resolution.proto index 265c2c5d6cc7..f6eb82c429ad 100644 --- a/proto/cymbal/resolution/v1/resolution.proto +++ b/proto/cymbal/resolution/v1/resolution.proto @@ -58,13 +58,10 @@ message Accepted {} message Done { // Serialized rust/cymbal Exception after frame resolution and exception // remapping. It replaces only the submitted exception item, not the full - // event or exception list. + // event or exception list. Resolved frames may carry a `release` object + // (the release bound to the symbol set that resolved them); older servers + // omit it and callers must treat a missing key as "no release". bytes resolved_exception_json = 1; - // Serialized JSON array of the releases bound to the symbol sets that - // resolved this exception's frames, deduped by release id. Empty when no - // resolved frame had a release. Callers must tolerate empty bytes (older - // servers do not set this field). - bytes releases_json = 2; } // Error is a terminal item failure. The kind enum is the shared control-flow diff --git a/rust/cymbal-proto/tests/contract.rs b/rust/cymbal-proto/tests/contract.rs index c3c54480e9a0..c30d1791b9b0 100644 --- a/rust/cymbal-proto/tests/contract.rs +++ b/rust/cymbal-proto/tests/contract.rs @@ -30,7 +30,6 @@ fn resolve_outcome_echoes_id_and_carries_done_error_or_retry() { id: 1, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: br#"{"type":"ResolvedError"}"#.to_vec(), - releases_json: br#"[{"version":"1.2.3"}]"#.to_vec(), })), }, ResolveOutcome { @@ -71,7 +70,7 @@ fn resolve_outcome_echoes_id_and_carries_done_error_or_retry() { assert!(matches!( &decoded[0].result, Some(resolve_outcome::Result::Done(done)) - if done.releases_json == br#"[{"version":"1.2.3"}]"# + if done.resolved_exception_json == br#"{"type":"ResolvedError"}"# )); assert!(matches!( decoded[1].result, @@ -101,25 +100,6 @@ fn resolve_outcome_echoes_id_and_carries_done_error_or_retry() { ); } -#[test] -fn done_without_releases_field_decodes_to_empty_bytes() { - // Skew compatibility both ways: a Done with no releases encodes byte-identical to the - // pre-`releases_json` message (prost omits default fields), and decoding such a message — - // what an older server sends — yields empty bytes, which callers must treat as "no releases". - let without_releases = Done { - resolved_exception_json: br#"{"type":"ResolvedError"}"#.to_vec(), - releases_json: Vec::new(), - }; - - let decoded = Done::decode(without_releases.encode_to_vec().as_slice()).unwrap(); - - assert_eq!( - decoded.resolved_exception_json, - br#"{"type":"ResolvedError"}"# - ); - assert!(decoded.releases_json.is_empty()); -} - #[test] fn subscribe_request_round_trips_caller_hint_and_identity() { let request = SubscribeRequest { diff --git a/rust/cymbal/docs/compatibility.md b/rust/cymbal/docs/compatibility.md index 87109dc68771..e1597ec74ba4 100644 --- a/rust/cymbal/docs/compatibility.md +++ b/rust/cymbal/docs/compatibility.md @@ -29,7 +29,7 @@ No generated type changes are needed: - Unsampled events use the local exception and frame resolvers. - Sampled remote events are flattened into exception-level `ResolveItem`s. Items are grouped by their first symbol-set reference when available, with a per-team fallback, then submitted over per-endpoint bidirectional `Resolve` streams. - Resolver-specific context is carried in `ResolveItem.metadata` as JSON bytes. The native symbolication convention uses a `debug_images_json` key. -- `Done.releases_json` carries the releases bound to the symbol sets that resolved the item's frames, as a JSON array deduped by release id. Empty bytes means no releases (also what servers predating the field send); `Frame.release` itself is `#[serde(skip)]` and never crosses the wire. +- Resolved frames in `Done.resolved_exception_json` may carry a `release` object: the release bound to the symbol set that resolved the frame. The key is omitted when a frame has none (also what older servers send for every frame), and callers must treat a missing key as "no release". The processing side strips `Frame.release` before any clickhouse-bound serialization. - Per-item `ResolveOutcome.Error.kind` is the control-flow surface. `ERROR_KIND_OVERLOADED` is result-only backpressure and triggers item reroute. Accepted items emit `ResolveOutcome.Accepted` before their terminal outcome; cymbal releases its routing permit on that acceptance signal. If `CYMBAL_REMOTE_RESOLUTION_OVERLOAD_EJECTION_MS` is non-zero, the overloaded endpoint is also temporarily excluded from new routing in that cymbal process. Repeated overloads double that cooldown up to `CYMBAL_REMOTE_RESOLUTION_OVERLOAD_EJECTION_MAX_MS`, and a quiet `CYMBAL_REMOTE_RESOLUTION_OVERLOAD_EJECTION_DECAY_MS` window resets it. `CYMBAL_REMOTE_RESOLUTION_ROUTING_JITTER` controls how much routing flattens across the load-adjusted rendezvous-ranked candidate list (`0.0` strict top load-adjusted candidate, `1.0` load-weighted across candidates). `LoadEvent` carries endpoint freshness/draining state plus `in_flight` / `max_in_flight` as a soft routing load signal. This means Node request chunking limits protect cymbal's public HTTP body size, while cymbal's private gRPC path owns exception-level routing, reroute depth, and overload handling. diff --git a/rust/cymbal/src/core/symbolication/symbol/records.rs b/rust/cymbal/src/core/symbolication/symbol/records.rs index 0a5f3c5bf609..c9a7a72fd24e 100644 --- a/rust/cymbal/src/core/symbolication/symbol/records.rs +++ b/rust/cymbal/src/core/symbolication/symbol/records.rs @@ -108,6 +108,12 @@ impl ErrorTrackingStackFrame { } else { None }; + // Stored contents never include the release: `load_all` re-joins it via the symbol set so + // a release (re)bind takes effect on the next load instead of going stale in cache rows. + let mut contents = serde_json::to_value(&self.contents)?; + if let Some(object) = contents.as_object_mut() { + object.remove("release"); + } sqlx::query!( r#" INSERT INTO posthog_errortrackingstackframe (raw_id, part, team_id, created_at, symbol_set_id, contents, resolved, id, context) @@ -124,7 +130,7 @@ impl ErrorTrackingStackFrame { self.id.team_id, self.created_at, self.symbol_set_id, - serde_json::to_value(&self.contents)?, + contents, self.resolved, Uuid::now_v7(), context, diff --git a/rust/cymbal/src/core/symbolication/symbol_store/saving.rs b/rust/cymbal/src/core/symbolication/symbol_store/saving.rs index 32ee989b62cb..ad782dc1681b 100644 --- a/rust/cymbal/src/core/symbolication/symbol_store/saving.rs +++ b/rust/cymbal/src/core/symbolication/symbol_store/saving.rs @@ -34,7 +34,7 @@ const NEGATIVE_CACHE_MAX_WEIGHT: u64 = 64 * 1024 * 1024; // We truncate the reference to resolve an issue with the maximum size in a BTRee index on Postgres // TODO: update model to use a hash of the reference instead -fn truncate_ref(s: &str) -> &str { +pub(crate) fn truncate_ref(s: &str) -> &str { if s.len() <= MAX_REF_BYTES { return s; } diff --git a/rust/cymbal/src/core/types/frames/mod.rs b/rust/cymbal/src/core/types/frames/mod.rs index ce35c6401c4b..22bc1968059e 100644 --- a/rust/cymbal/src/core/types/frames/mod.rs +++ b/rust/cymbal/src/core/types/frames/mod.rs @@ -173,10 +173,11 @@ pub struct Frame { // use in the frontend #[serde(skip)] pub context: Option, - // The release bound to the symbol set that resolved this frame. Never serialized: it must not - // reach the clickhouse-bound event JSON, and the remote resolution response carries releases - // in its own `releases_json` sidecar field instead of inside the frame. - #[serde(skip)] + // The release bound to the symbol set that resolved this frame. Serializable so it crosses the + // resolution-service wire inside the frame JSON, but it must not reach the clickhouse-bound + // event JSON — `into_resolved` strips it after `$exception_release` selection — and the PG + // frame cache re-joins it at load instead of trusting a stored copy (see records.rs). + #[serde(skip_serializing_if = "Option::is_none", default)] pub release: Option, } diff --git a/rust/cymbal/src/core/types/frames/releases.rs b/rust/cymbal/src/core/types/frames/releases.rs index 9915e17cadc6..78404252176c 100644 --- a/rust/cymbal/src/core/types/frames/releases.rs +++ b/rust/cymbal/src/core/types/frames/releases.rs @@ -7,10 +7,12 @@ use sha2::{Digest, Sha512}; use sqlx::Executor; use uuid::Uuid; +use crate::symbolication::symbol_store::saving::truncate_ref; + use super::Frame; -// Serialized only on the internal resolution-service wire (`Done.releases_json`), never into the -// clickhouse-bound event JSON — `Frame.release` stays `#[serde(skip)]`. +// Serialized only on the internal resolution-service wire, inside each resolved frame's JSON — +// never into the clickhouse-bound event JSON, which `into_resolved` strips `Frame.release` from. #[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)] pub struct ReleaseRecord { pub id: Uuid, @@ -85,6 +87,9 @@ impl ReleaseRecord { where E: Executor<'c, Database = sqlx::Postgres>, { + // Stored refs are truncated to MAX_REF_BYTES by SymbolSetRecord::load/save; match on the + // same truncated value or long refs (e.g. >2KB JS source URLs) never join. + let symbol_set_ref = truncate_ref(symbol_set_ref); let row = sqlx::query_as!( Self, r#" diff --git a/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs b/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs index 558246d9fb18..ff9ac7c89ed2 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/remote/mux.rs @@ -427,7 +427,6 @@ mod tests { id: item.id, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: item.exception_json, - releases_json: Vec::new(), })), }; tx.send(Ok(outcome)).await.expect("test receiver is alive"); diff --git a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs index 2e77f89ac39f..3ee12ce127f9 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver.rs @@ -8,7 +8,6 @@ use tokio::sync::Semaphore; use crate::{ error::UnhandledError, - frames::releases::ReleaseRecord, stages::{ pipeline::ParsedPipelineItem, resolution::{ @@ -167,9 +166,6 @@ struct ResolvedRemoteItem { event_slot: usize, exception_slot: usize, exception: Exception, - /// Releases bound to the symbol sets that resolved this exception's frames, from the - /// response's `releases_json` sidecar — `Frame.release` itself does not survive the wire. - releases: Vec, } // ───────────────────────────────────────────────────────────────────────────── @@ -237,7 +233,6 @@ async fn resolve_remote_events( item.event_slot, item.exception_slot ))); } - event_slot.evt.add_frame_releases(item.releases); } event_slots diff --git a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs index 0e83c48d8750..8065fe891280 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/remote/resolver/retry.rs @@ -17,7 +17,6 @@ use cymbal_proto::cymbal::resolution::v1::{resolve_outcome, ErrorKind, ResolveOu use tonic::Status; use crate::error::UnhandledError; -use crate::frames::releases::ReleaseRecord; use crate::metric_consts::{ REMOTE_RESOLUTION_ERROR_KINDS, REMOTE_RESOLUTION_LATENCY, REMOTE_RESOLUTION_OVERLOAD_ESCALATIONS, REMOTE_RESOLUTION_REQUESTS, @@ -152,17 +151,13 @@ pub(super) async fn resolve_work_item( }; match decision { - ItemDecision::Done { - exception, - releases, - } => { + ItemDecision::Done(exception) => { metrics::counter!(REMOTE_RESOLUTION_REQUESTS, "outcome" => "ok").increment(1); record_reroute_depth("ok", attempts_used); return Ok(ResolvedRemoteItem { event_slot: work_item.event_slot, exception_slot: work_item.exception_slot, exception, - releases, }); } ItemDecision::Overloaded(message) => { @@ -280,10 +275,7 @@ fn single_outcome( #[derive(Debug)] enum ItemDecision { - Done { - exception: Exception, - releases: Vec, - }, + Done(Exception), Overloaded(String), Retry { message: String, @@ -311,24 +303,7 @@ fn classify_outcome( format!("invalid_done_payload: failed to parse resolved exception: {err}"), ) })?; - // Empty bytes means an older server that predates the field, or no frame resolved - // with a release — both are simply "no releases". - let releases = if done.releases_json.is_empty() { - Vec::new() - } else { - serde_json::from_slice::>(&done.releases_json).map_err( - |err| { - terminal_item_error( - work_item.token, - format!("invalid_done_payload: failed to parse frame releases: {err}"), - ) - }, - )? - }; - Ok(ItemDecision::Done { - exception, - releases, - }) + Ok(ItemDecision::Done(exception)) } resolve_outcome::Result::Retry(retry) => { let retry_after = (retry.retry_after_ms > 0) @@ -450,32 +425,33 @@ fn record_reroute_depth(outcome: &'static str, attempts_used: u32) { mod tests { use cymbal_proto::cymbal::resolution::v1::{Done, Error, Retry}; + use crate::frames::releases::ReleaseRecord; + use super::*; #[test] fn classify_outcome_parses_done_exception() { let work_item = work_item(7); - // Empty `releases_json` is what a server predating the field sends; it must decode as - // "no releases", not an error. let outcome = ResolveOutcome { id: 7, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: serde_json::to_vec(&exception("Resolved")) .expect("valid exception"), - releases_json: Vec::new(), })), }; let decision = classify_outcome(&work_item, outcome).expect("done outcome"); assert!(matches!( decision, - ItemDecision::Done { exception, releases } - if exception.exception_type == "Resolved" && releases.is_empty() + ItemDecision::Done(exception) if exception.exception_type == "Resolved" )); } + // The frame-derived release crosses the wire inside the frame JSON; a frame without the + // key (an older server, or no release bound) must parse to `None`, and one with it must + // land on `Frame.release`. #[test] - fn classify_outcome_parses_done_releases_sidecar() { + fn classify_outcome_parses_frame_releases_from_the_exception_json() { let release = ReleaseRecord { id: uuid::Uuid::now_v7(), team_id: 42, @@ -485,36 +461,29 @@ mod tests { project: "my-app".to_string(), metadata: None, }; + let mut with_release = exception("Resolved"); + with_release.stack = Some(crate::types::Stacktrace::Resolved { + frames: vec![frame(Some(release.clone())), frame(None)], + }); + let work_item = work_item(7); let outcome = ResolveOutcome { id: 7, result: Some(resolve_outcome::Result::Done(Done { - resolved_exception_json: serde_json::to_vec(&exception("Resolved")) + resolved_exception_json: serde_json::to_vec(&with_release) .expect("valid exception"), - releases_json: serde_json::to_vec(&vec![release.clone()]).expect("valid releases"), })), }; let decision = classify_outcome(&work_item, outcome).expect("done outcome"); - assert!(matches!( - decision, - ItemDecision::Done { releases, .. } if releases == vec![release] - )); - } - - #[test] - fn classify_outcome_rejects_malformed_releases_sidecar() { - let work_item = work_item(7); - let outcome = ResolveOutcome { - id: 7, - result: Some(resolve_outcome::Result::Done(Done { - resolved_exception_json: serde_json::to_vec(&exception("Resolved")) - .expect("valid exception"), - releases_json: b"not json".to_vec(), - })), + let ItemDecision::Done(parsed) = decision else { + panic!("expected done decision"); }; - - assert!(classify_outcome(&work_item, outcome).is_err()); + let Some(crate::types::Stacktrace::Resolved { frames }) = parsed.stack else { + panic!("expected resolved stack"); + }; + assert_eq!(frames[0].release, Some(release)); + assert_eq!(frames[1].release, None); } #[test] @@ -609,4 +578,26 @@ mod tests { stack: None, } } + + fn frame(release: Option) -> crate::frames::Frame { + crate::frames::Frame { + frame_id: common_types::error_tracking::FrameId::placeholder(), + mangled_name: "f".to_string(), + line: None, + column: None, + source: None, + module: None, + in_app: true, + resolved_name: None, + lang: "javascript".to_string(), + resolved: true, + resolve_failure: None, + synthetic: false, + suspicious: false, + junk_drawer: None, + code_variables: None, + context: None, + release, + } + } } diff --git a/rust/cymbal/src/modes/processing/types/exception_event.rs b/rust/cymbal/src/modes/processing/types/exception_event.rs index 49868a51c50e..d9352a0772ae 100644 --- a/rust/cymbal/src/modes/processing/types/exception_event.rs +++ b/rust/cymbal/src/modes/processing/types/exception_event.rs @@ -29,11 +29,6 @@ pub struct Parsed { /// The release resolved from the event's `$release_id` or mobile app metadata, if any. Set by /// `EventReleaseResolver` and emitted as `$exception_release` at `into_resolved`. pub(crate) event_release: Option, - /// Releases bound to the symbol sets that resolved this event's frames, accumulated from the - /// remote resolution response sidecar. The local resolution path instead leaves them on - /// `Frame.release`; both sources are merged at `into_resolved`, where the latest one becomes - /// the `$exception_release` fallback when `event_release` is unset. - pub(crate) frame_releases: Vec, } #[derive(Debug, Clone)] @@ -52,18 +47,13 @@ impl ResolvedMetadata { fn from_exception_list( exception_list: &ExceptionList, event_release: Option<&ReleaseRecord>, - frame_releases: Vec, ) -> Self { // The event-level release (`$release_id` / mobile app-metadata hash) is authoritative; // frame-derived releases only fill in when it resolved nothing, picking the latest so an // event whose stack mixes chunks from several releases reports the newest one. let release = event_release .cloned() - .or_else(|| { - let mut candidates = frame_releases; - candidates.extend(exception_list.get_frame_releases()); - ReleaseRecord::latest(candidates) - }) + .or_else(|| ReleaseRecord::latest(exception_list.get_frame_releases())) .map(|release| release.to_info()); Self { @@ -235,16 +225,14 @@ impl ExceptionEvent { self.state.event_release = release; } - pub(crate) fn add_frame_releases(&mut self, releases: Vec) { - self.state.frame_releases.extend(releases); - } - - pub(crate) fn into_resolved(self) -> ExceptionEvent { + pub(crate) fn into_resolved(mut self) -> ExceptionEvent { let metadata = ResolvedMetadata::from_exception_list( &self.exception_list, self.state.event_release.as_ref(), - self.state.frame_releases.clone(), ); + // `Frame.release` serializes on the resolution-service wire, but must never reach the + // clickhouse-bound serializations of the exception list; selection is done, so drop it. + self.exception_list.clear_frame_releases(); self.map_state(|state| Resolved { metadata, client_fingerprint: state.client_fingerprint, @@ -606,7 +594,6 @@ impl TryFrom for ExceptionEvent { legacy_order_exception_list, legacy_order_resolved: None, event_release: None, - frame_releases: Vec::new(), }, }) } @@ -791,7 +778,6 @@ mod tests { let metadata = ResolvedMetadata::from_exception_list( &ExceptionList::default(), Some(&release_record("hash-abc")), - Vec::new(), ); assert!(metadata.release.is_some()); } @@ -800,8 +786,7 @@ mod tests { fn missing_event_release_leaves_the_release_unset() { // Without an event-level release and without any frame-derived candidate there is nothing // to emit. - let metadata = - ResolvedMetadata::from_exception_list(&ExceptionList::default(), None, Vec::new()); + let metadata = ResolvedMetadata::from_exception_list(&ExceptionList::default(), None); assert!(metadata.release.is_none()); } @@ -811,47 +796,64 @@ mod tests { // newer; the fallback only fills a gap, it never overrides. let event_release = release_record_at("event-hash", 100); let newer_frame_release = release_record_at("frame-hash", 5_000); + let exception_list = + exception_list_with_frames(vec![frame_with_release(Some(newer_frame_release))]); - let metadata = ResolvedMetadata::from_exception_list( - &ExceptionList::default(), - Some(&event_release), - vec![newer_frame_release], - ); + let metadata = + ResolvedMetadata::from_exception_list(&exception_list, Some(&event_release)); let expected = serde_json::to_value(event_release.to_info()).unwrap(); assert_eq!(serde_json::to_value(&metadata.release).unwrap(), expected); } #[test] - fn frame_release_fallback_picks_the_latest_across_both_sources() { - // Without an event-level release, the fallback considers the remote sidecar and the - // releases local symbolication left on the frames, and picks the most recently created. - let sidecar_release = release_record_at("sidecar-hash", 100); - let latest_frame_release = release_record_at("frame-hash", 5_000); - let exception_list = exception_list_with_frames(vec![frame_with_release(Some( - latest_frame_release.clone(), - ))]); - - let metadata = ResolvedMetadata::from_exception_list( - &exception_list, - None, - vec![sidecar_release.clone()], - ); + fn into_resolved_strips_releases_from_frames_after_selection() { + // `Frame.release` serializes (for the resolution-service wire), so if `into_resolved` + // stopped stripping it, release payloads would leak into every clickhouse-bound + // serialization of the exception list. + let release = release_record_at("frame-hash", 5_000); + let parsed = ExceptionEvent { + uuid: Uuid::now_v7(), + team_id: 42, + timestamp: "2026-01-01T00:00:00Z".to_string(), + exception_list: exception_list_with_frames(vec![frame_with_release(Some(release))]), + debug_images: vec![], + props: HashMap::new(), + proposed_issue_name: None, + proposed_issue_description: None, + state: Parsed { + client_fingerprint: None, + legacy_order_exception_list: None, + legacy_order_resolved: None, + event_release: None, + }, + }; - let expected = serde_json::to_value(latest_frame_release.to_info()).unwrap(); - assert_eq!(serde_json::to_value(&metadata.release).unwrap(), expected); + let resolved = parsed.into_resolved(); - // And the other way around: a newer sidecar release beats an older frame-attached one. - let exception_list = - exception_list_with_frames(vec![frame_with_release(Some(sidecar_release))]); - let newer_sidecar = release_record_at("sidecar-hash-2", 9_000); - let metadata = ResolvedMetadata::from_exception_list( - &exception_list, - None, - vec![newer_sidecar.clone()], + assert!(resolved.metadata().release.is_some(), "selection ran first"); + let list_json = serde_json::to_string(resolved.exception_list()).unwrap(); + assert!( + !list_json.contains("release"), + "clickhouse-bound exception list must not carry frame releases: {list_json}" ); + } - let expected = serde_json::to_value(newer_sidecar.to_info()).unwrap(); + #[test] + fn frame_release_fallback_picks_the_latest() { + // Without an event-level release, the fallback picks the most recently created release + // across the frames, regardless of frame order. + let older_release = release_record_at("older-hash", 100); + let latest_release = release_record_at("latest-hash", 5_000); + let exception_list = exception_list_with_frames(vec![ + frame_with_release(Some(older_release)), + frame_with_release(None), + frame_with_release(Some(latest_release.clone())), + ]); + + let metadata = ResolvedMetadata::from_exception_list(&exception_list, None); + + let expected = serde_json::to_value(latest_release.to_info()).unwrap(); assert_eq!(serde_json::to_value(&metadata.release).unwrap(), expected); } diff --git a/rust/cymbal/src/modes/processing/types/mod.rs b/rust/cymbal/src/modes/processing/types/mod.rs index d5dbeb7b0a55..2934527b0b6d 100644 --- a/rust/cymbal/src/modes/processing/types/mod.rs +++ b/rust/cymbal/src/modes/processing/types/mod.rs @@ -94,12 +94,23 @@ impl ExceptionList { .unwrap_or(false) } - /// Releases attached in-memory to this list's frames by local symbolication. Frames that came - /// back from the remote resolution service never carry one (`Frame.release` is not - /// serialized); their releases arrive via the response sidecar instead. + /// Releases attached to this list's frames — by local symbolication directly, or deserialized + /// from the remote resolution response (`Frame.release` serializes on that wire). pub fn get_frame_releases(&self) -> Vec { ReleaseRecord::collect_from_frames(self.get_frames_iter()) } + + /// Drops `Frame.release` from every resolved frame. Called once `$exception_release` + /// selection is done, so clickhouse-bound serializations of this list never carry it. + pub fn clear_frame_releases(&mut self) { + for exception in self.0.iter_mut() { + if let Some(Stacktrace::Resolved { frames }) = exception.stack.as_mut() { + for frame in frames { + frame.release = None; + } + } + } + } } /// Untrusted exception properties accepted from ClickHouse and SDK event payloads. diff --git a/rust/cymbal/src/modes/resolution/README.md b/rust/cymbal/src/modes/resolution/README.md index 83f5247fc5e9..6ae51ba0b95c 100644 --- a/rust/cymbal/src/modes/resolution/README.md +++ b/rust/cymbal/src/modes/resolution/README.md @@ -46,7 +46,7 @@ events ──HTTP─────▶│ cymbal The contract is intentionally split across two streams: -- **`Resolve`** is bidirectional work traffic. The caller sends independent `ResolveItem`s, each with a per-stream id, `team_id`, serialized exception JSON, JSON `metadata` bytes, and an item deadline. The server emits an `Accepted` outcome when it admits an item, then exactly one terminal `ResolveOutcome` with the same id: `Done`, `Retry`, or `Error`. A `Done` carries the resolved exception JSON plus a `releases_json` sidecar: the releases bound to the symbol sets that resolved the exception's frames, deduped by release id. `Frame.release` is never serialized, so the sidecar is the only way releases cross this boundary; callers must treat empty bytes as "no releases" (older servers do not set the field). +- **`Resolve`** is bidirectional work traffic. The caller sends independent `ResolveItem`s, each with a per-stream id, `team_id`, serialized exception JSON, JSON `metadata` bytes, and an item deadline. The server emits an `Accepted` outcome when it admits an item, then exactly one terminal `ResolveOutcome` with the same id: `Done`, `Retry`, or `Error`. A `Done` carries the resolved exception JSON; each resolved frame may carry a `release` object (the release bound to the symbol set that resolved it), with the key omitted when there is none — which is also what older servers send for every frame. - **`Subscribe`** is endpoint freshness, draining, and soft load state. The cymbal-side `EndpointPool` opens one long-lived stream per pod and treats the latest `LoadEvent` as a freshness snapshot plus an `in_flight` / `max_in_flight` routing bias. `LoadEvent` does not carry overload state or suggested batch sizing. `Error.kind` is the shared control-flow surface: diff --git a/rust/cymbal/src/modes/resolution/service.rs b/rust/cymbal/src/modes/resolution/service.rs index 4bfe184f6bdd..5cc999daee66 100644 --- a/rust/cymbal/src/modes/resolution/service.rs +++ b/rust/cymbal/src/modes/resolution/service.rs @@ -68,7 +68,7 @@ impl CymbalResolutionService { symbol_resolution_limiter: self.symbol_resolution_limiter.clone(), // Event-level release resolution (`$release_id` / app-metadata hash) runs on the // processing side, so no release pool or cache is needed here. The frame-derived - // releases this server returns in the `Done` sidecar come from the symbol-set join + // releases this server returns on the resolved frames come from the symbol-set join // inside the symbol resolver, which uses its own pool. posthog_pool: None, release_cache: ReleaseCache::disabled(), diff --git a/rust/cymbal/src/modes/resolution/service/resolve.rs b/rust/cymbal/src/modes/resolution/service/resolve.rs index 9f1225afd420..d54dbd792887 100644 --- a/rust/cymbal/src/modes/resolution/service/resolve.rs +++ b/rust/cymbal/src/modes/resolution/service/resolve.rs @@ -8,12 +8,11 @@ use tonic::{Status, Streaming}; use tracing::{debug, warn}; use crate::error::UnhandledError; -use crate::frames::releases::ReleaseRecord; use crate::langs::native::DebugImage; use crate::stages::resolution::exception::ExceptionResolver; use crate::stages::resolution::frame::FrameResolver; use crate::stages::resolution::ResolutionStage; -use crate::types::{Exception, Stacktrace}; +use crate::types::Exception; use cymbal_proto::cymbal::resolution::v1::{ resolve_outcome, Accepted, Done, Error as ItemError, ResolveItem, ResolveOutcome, }; @@ -151,10 +150,9 @@ async fn process_item( let (result, outcome, kind) = match tokio::time::timeout(deadline, resolve_item(&stage, &item)).await { - Ok(Ok(resolved)) => ( + Ok(Ok(exception_json)) => ( resolve_outcome::Result::Done(Done { - resolved_exception_json: resolved.exception_json, - releases_json: resolved.releases_json, + resolved_exception_json: exception_json, }), "done", "ok", @@ -232,15 +230,7 @@ enum ItemFailure { Unhandled(String), } -struct ResolvedItemPayload { - exception_json: Vec, - releases_json: Vec, -} - -async fn resolve_item( - stage: &ResolutionStage, - item: &ResolveItem, -) -> Result { +async fn resolve_item(stage: &ResolutionStage, item: &ResolveItem) -> Result, ItemFailure> { let exception: Exception = serde_json::from_slice(&item.exception_json) .map_err(|e| ItemFailure::InvalidPayload(format!("invalid exception_json: {e}")))?; @@ -255,31 +245,10 @@ async fn resolve_item( ResolveOneError::Unhandled(err) => ItemFailure::Unhandled(err), })?; - let releases_json = frame_releases_json(&resolved)?; - let exception_json = serde_json::to_vec(&resolved) - .map_err(|e| ItemFailure::Unhandled(format!("serialize resolved exception: {e}")))?; - - Ok(ResolvedItemPayload { - exception_json, - releases_json, - }) -} - -// `Frame.release` is `#[serde(skip)]`, so the releases the symbol-set join attached during -// resolution would be lost in `resolved_exception_json`; they cross the wire in this sidecar -// instead. Empty bytes (not an empty JSON array) when nothing resolved, matching what an older -// server sends. -fn frame_releases_json(exception: &Exception) -> Result, ItemFailure> { - let frames = match &exception.stack { - Some(Stacktrace::Resolved { frames }) => frames.as_slice(), - _ => &[], - }; - let releases = ReleaseRecord::collect_from_frames(frames.iter()); - if releases.is_empty() { - return Ok(Vec::new()); - } - serde_json::to_vec(&releases) - .map_err(|e| ItemFailure::Unhandled(format!("serialize frame releases: {e}"))) + // Resolved frames carry their symbol set's release inline (`Frame.release` serializes on + // this wire); the processing side strips it before anything clickhouse-bound. + serde_json::to_vec(&resolved) + .map_err(|e| ItemFailure::Unhandled(format!("serialize resolved exception: {e}"))) } fn debug_images_from_metadata(metadata: &[u8]) -> Result, ItemFailure> { diff --git a/rust/cymbal/tests/common/mod.rs b/rust/cymbal/tests/common/mod.rs index 670143c3aa08..30d0bb40c522 100644 --- a/rust/cymbal/tests/common/mod.rs +++ b/rust/cymbal/tests/common/mod.rs @@ -259,7 +259,6 @@ fn done_outcome(item: &ResolveItem) -> ResolveOutcome { id: item.id, result: Some(resolve_outcome::Result::Done(Done { resolved_exception_json: item.exception_json.clone(), - releases_json: Vec::new(), })), } } diff --git a/rust/cymbal/tests/remote_resolution_parity.rs b/rust/cymbal/tests/remote_resolution_parity.rs index 58699dd4f099..2842ed2e0aa0 100644 --- a/rust/cymbal/tests/remote_resolution_parity.rs +++ b/rust/cymbal/tests/remote_resolution_parity.rs @@ -254,10 +254,10 @@ fn sample_raw_frame() -> RawFrame { .expect("valid raw frame") } -/// The frame-derived release reaches `$exception_release` identically on both paths, even though -/// it travels differently: in-memory on `Frame.release` locally, via the `releases_json` sidecar -/// remotely. The byte-for-byte exception-list comparison cannot see it (`Frame.release` is -/// serde-skipped), so it gets its own parity assertion. +/// The frame-derived release reaches `$exception_release` identically on both paths: in-memory on +/// `Frame.release` locally, serialized inside the frame JSON remotely. The byte-for-byte +/// exception-list comparison cannot see it (`into_resolved` strips releases from frames after +/// selection, on both paths), so it gets its own parity assertion. #[tokio::test] async fn local_and_remote_stages_produce_identical_exception_release() { let resolver: Arc = Arc::new(ReleaseAttachingResolver { diff --git a/rust/cymbal/tests/resolution_service_tests.rs b/rust/cymbal/tests/resolution_service_tests.rs index 14b99f59b49c..924d7bd953c9 100644 --- a/rust/cymbal/tests/resolution_service_tests.rs +++ b/rust/cymbal/tests/resolution_service_tests.rs @@ -344,24 +344,24 @@ async fn raw_frames_are_resolved_into_done_payload() { } #[tokio::test] -async fn frame_releases_are_emitted_in_the_done_sidecar_and_kept_out_of_the_exception() { +async fn frame_releases_are_serialized_inside_the_resolved_frames() { let release = ReleaseRecord { id: uuid::Uuid::from_u128(7), team_id: 123, - hash_id: "sidecar-hash".to_string(), + hash_id: "wire-hash".to_string(), created_at: chrono::DateTime::from_timestamp(1_700_000_000, 0).unwrap(), version: "9.9.9".to_string(), project: "my-app".to_string(), metadata: None, }; let raw_frame = sample_raw_frame(); - // Two resolved frames carrying the same release: the sidecar must dedupe by release id. + // One frame with a release, one without: the release must ride its own frame's JSON and a + // missing key (what an older server sends for every frame) must deserialize to `None`. let mut first = sample_resolved_frame(&raw_frame); first.frame_id = raw_frame.frame_id(123, 0, &[]); first.release = Some(release.clone()); let mut second = sample_resolved_frame(&raw_frame); second.frame_id = raw_frame.frame_id(123, 1, &[]); - second.release = Some(release.clone()); let service = make_service(FakeResolver { fail_unhandled: false, resolved_frames: vec![first, second], @@ -377,41 +377,22 @@ async fn frame_releases_are_emitted_in_the_done_sidecar_and_kept_out_of_the_exce panic!("expected Done outcome, got {:?}", outcomes[0]); }; - let releases: Vec = - serde_json::from_slice(&done.releases_json).expect("valid releases sidecar"); - assert_eq!(releases, vec![release]); + let resolved: Exception = + serde_json::from_slice(&done.resolved_exception_json).expect("valid resolved exception"); + let Some(Stacktrace::Resolved { frames }) = resolved.stack else { + panic!("raw stack must be replaced with resolved frames"); + }; + assert_eq!(frames[0].release, Some(release)); + assert_eq!(frames[1].release, None); - // `Frame.release` must stay out of the serialized exception: the sidecar is the only place - // releases cross the wire, and the frame JSON shape doubles as clickhouse output. - let resolved: serde_json::Value = + let raw: serde_json::Value = serde_json::from_slice(&done.resolved_exception_json).expect("valid resolved exception"); - let frames = resolved["stacktrace"]["frames"] + let frames_json = raw["stacktrace"]["frames"] .as_array() .expect("resolved frames present"); - assert!(frames.iter().all(|frame| frame.get("release").is_none())); -} - -#[tokio::test] -async fn done_sidecar_is_empty_bytes_when_no_frame_has_a_release() { - let raw_frame = sample_raw_frame(); - let mut resolver_frame = sample_resolved_frame(&raw_frame); - resolver_frame.frame_id = raw_frame.frame_id(123, 99, &[]); - let service = make_service(FakeResolver { - fail_unhandled: false, - resolved_frames: vec![resolver_frame], - }); - let mut exc = raw_exception("RuntimeError"); - exc.stack = Some(Stacktrace::Raw { - frames: vec![raw_frame], - }); - - let outcomes = resolve_items(service, vec![make_item(1, &exc)]).await; - let resolve_outcome::Result::Done(done) = outcome_result(&outcomes[0]) else { - panic!("expected Done outcome, got {:?}", outcomes[0]); - }; - - // Empty bytes, not `[]`: byte-identical to what a server predating the field sends. - assert!(done.releases_json.is_empty()); + assert!(frames_json[0].get("release").is_some()); + // `skip_serializing_if` keeps release-less frames byte-identical to the old wire shape. + assert!(frames_json[1].get("release").is_none()); } #[tokio::test] From 320d38739f3b7e7d53f3459ba1ce1a2109198356 Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Tue, 4 Aug 2026 11:16:03 +0200 Subject: [PATCH 4/9] feat(error-tracking): include release id in exception release snapshot The $exception_release payload was a pure point-in-time copy, so a release edited later via the public API could never be reconciled with events already written. Carrying the release row id keeps the snapshot cheap to read while letting consumers re-fetch current values by id. Also corrects two stale doc comments: releases are not immutable (the public API can update or delete them; the TTL bounds cache staleness), and there is no per-frame fallback when event-level resolution misses. Co-Authored-By: Claude Fable 5 --- rust/cymbal/src/core/types/frames/releases.rs | 6 +++++- .../processing/stages/resolution/event_release.rs | 12 ++++++------ .../src/modes/processing/types/exception_event.rs | 5 +++-- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/rust/cymbal/src/core/types/frames/releases.rs b/rust/cymbal/src/core/types/frames/releases.rs index e64c6c0edea8..07181ba5a83f 100644 --- a/rust/cymbal/src/core/types/frames/releases.rs +++ b/rust/cymbal/src/core/types/frames/releases.rs @@ -16,9 +16,12 @@ pub struct ReleaseRecord { pub metadata: Option, } -// The info, as written to clickhouse at the exception level. +// The info, as written to clickhouse at the exception level. The scalar fields are a +// point-in-time snapshot; `id` references the release row so consumers can re-fetch the +// current values if the release is later edited via the API. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ReleaseInfo { + id: Uuid, version: String, project: String, timestamp: DateTime, @@ -73,6 +76,7 @@ impl ReleaseRecord { pub fn to_info(&self) -> ReleaseInfo { ReleaseInfo { + id: self.id, project: self.project.clone(), version: self.version.clone(), timestamp: self.created_at, diff --git a/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs index 9395842fc966..f84826a78fad 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs @@ -18,10 +18,10 @@ use crate::{ /// Per-worker cache for event-level release resolution. Both lookups run once per exception event /// on the ingestion hot path, so without this a mobile app that never bound a release re-queries -/// Postgres on every event it sends (the common negative case). A release row is immutable once -/// the CLI creates it, so a positive hit never goes stale; caching the negative result too is what -/// removes the per-event query, and the TTL bounds how long a miss lingers after a later dSYM -/// upload creates the release. +/// Postgres on every event it sends (the common negative case). The CLI never mutates a release +/// after creating it, but the public API can update or delete one, so a positive hit can go stale; +/// the TTL bounds that staleness, and also how long a miss lingers after a later dSYM upload +/// creates the release. Caching the negative result too is what removes the per-event query. /// /// The two lookups key on different things — a release-row id for web builds, a reconstructed /// content hash for mobile builds — so they get separate caches. `try_get_with` coalesces @@ -136,8 +136,8 @@ fn record_cache_outcome(cache_type: &'static str, cache_miss: bool) { /// `$app_version`, and `$app_build`, which the CLI hashed into the release when it uploaded the /// dSYMs. We reconstruct that hash and look the release up by it. /// -/// When neither resolves, the event release stays unset and the pipeline falls back to the -/// per-frame symbol-set join for legacy events. +/// When neither resolves, the event release stays unset and `$exception_release` is omitted; +/// there is no per-frame fallback. #[derive(Clone, Default)] pub struct EventReleaseResolver; diff --git a/rust/cymbal/src/modes/processing/types/exception_event.rs b/rust/cymbal/src/modes/processing/types/exception_event.rs index db31b2239e67..f1d494bf7fcf 100644 --- a/rust/cymbal/src/modes/processing/types/exception_event.rs +++ b/rust/cymbal/src/modes/processing/types/exception_event.rs @@ -752,12 +752,13 @@ mod tests { description: None, created_at: chrono::Utc::now(), }; - let expected = serde_json::to_value(release_record("hash-abc").to_info()).unwrap(); + let record = release_record("hash-abc"); + let expected = serde_json::to_value(record.to_info()).unwrap(); // A resolved release surfaces as `$exception_release` on both the grouping-rule projection // and the derived wire form. let mut resolved = resolved_event(); - resolved.state.metadata.release = Some(release_record("hash-abc").to_info()); + resolved.state.metadata.release = Some(record.to_info()); let grouping = resolved.grouping_rule_properties(); assert_eq!(grouping["$exception_release"], expected); let fingerprinted = From 899d17912ecd56c54685abc9620508d664946dbc Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Tue, 4 Aug 2026 12:13:11 +0200 Subject: [PATCH 5/9] refactor(error-tracking): fold release resolution into ResolutionStage A separate EventReleaseStage left ResolutionStage emitting still-Parsed events, which made the stage name lie about its output. Release resolution is resolution: run the operator inside ResolutionStage after resolve_batch, restoring Parsed -> Resolved as the stage contract while keeping the release lookup downstream of frame resolution for the future legacy-event fallback. Co-Authored-By: Claude Fable 5 --- rust/cymbal/src/core/metric_consts.rs | 1 - .../src/modes/processing/app_context.rs | 2 +- .../cymbal/src/modes/processing/stages/mod.rs | 1 - .../src/modes/processing/stages/pipeline.rs | 6 +-- .../stages/{ => resolution}/event_release.rs | 54 ++++--------------- .../modes/processing/stages/resolution/mod.rs | 19 +++++-- rust/cymbal/tests/common/mod.rs | 17 ++++-- rust/cymbal/tests/remote_resolution.rs | 9 +++- 8 files changed, 45 insertions(+), 64 deletions(-) rename rust/cymbal/src/modes/processing/stages/{ => resolution}/event_release.rs (84%) diff --git a/rust/cymbal/src/core/metric_consts.rs b/rust/cymbal/src/core/metric_consts.rs index 28292c754810..5a2e0f45d462 100644 --- a/rust/cymbal/src/core/metric_consts.rs +++ b/rust/cymbal/src/core/metric_consts.rs @@ -121,7 +121,6 @@ pub const LINKING_STAGE: &str = "cymbal_issue_processing_time"; pub const GROUPING_STAGE: &str = "cymbal_exception_grouping_stage"; pub const ALERTING_STAGE: &str = "cymbal_exception_alerting_stage"; pub const RATE_LIMITING_STAGE: &str = "cymbal_rate_limiting_stage"; -pub const EVENT_RELEASE_STAGE: &str = "cymbal_event_release_stage"; pub const RATE_LIMIT_OUTCOMES: &str = "cymbal_error_tracking_rate_limiter_outcomes"; pub const RATE_LIMIT_FAIL_OPEN: &str = "cymbal_error_tracking_rate_limiter_fail_open"; pub const RATE_LIMIT_METRIC_EMIT: &str = "cymbal_error_tracking_rate_limiter_metric_emit"; diff --git a/rust/cymbal/src/modes/processing/app_context.rs b/rust/cymbal/src/modes/processing/app_context.rs index 5361cb1a89fc..028366fc0ed9 100644 --- a/rust/cymbal/src/modes/processing/app_context.rs +++ b/rust/cymbal/src/modes/processing/app_context.rs @@ -13,8 +13,8 @@ use crate::{ core::config::build_pg_pool, error::UnhandledError, modes::processing::config::{init_global_state, ProcessingConfig}, - stages::event_release::ReleaseCache, stages::rate_limiting::RedisRateLimiter, + stages::resolution::event_release::ReleaseCache, stages::resolution::remote::{ dns::TokioDnsResolver, pool::EndpointPool, resolver::RemoteResolutionContext, RemoteResolutionConfig, diff --git a/rust/cymbal/src/modes/processing/stages/mod.rs b/rust/cymbal/src/modes/processing/stages/mod.rs index 8dce5919b41d..610fa5fc6a3e 100644 --- a/rust/cymbal/src/modes/processing/stages/mod.rs +++ b/rust/cymbal/src/modes/processing/stages/mod.rs @@ -1,5 +1,4 @@ pub mod alerting; -pub mod event_release; pub mod grouping; pub mod http_pipeline; pub mod linking; diff --git a/rust/cymbal/src/modes/processing/stages/pipeline.rs b/rust/cymbal/src/modes/processing/stages/pipeline.rs index 682b0052b57a..67e6d705ba29 100644 --- a/rust/cymbal/src/modes/processing/stages/pipeline.rs +++ b/rust/cymbal/src/modes/processing/stages/pipeline.rs @@ -8,7 +8,6 @@ use crate::{ metric_consts::EXCEPTION_PROCESSING_PIPELINE, stages::{ alerting::AlertingStage, - event_release::EventReleaseStage, grouping::GroupingStage, linking::LinkingStage, post_processing::{PostProcessingHandler, PostProcessingStage}, @@ -56,12 +55,9 @@ impl Stage for ExceptionEventPipeline { async fn process(self, batch: Batch) -> StageResult { batch - // Resolve stack traces + // Resolve stack traces and the event-level release .apply_stage(ResolutionStage::from(&self.app_context)) .await? - // Resolve the event-level release and flip to Resolved - .apply_stage(EventReleaseStage::from(&self.app_context)) - .await? // Group events by fingerprint .apply_stage(GroupingStage::from(&self.app_context)) .await? diff --git a/rust/cymbal/src/modes/processing/stages/event_release.rs b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs similarity index 84% rename from rust/cymbal/src/modes/processing/stages/event_release.rs rename to rust/cymbal/src/modes/processing/stages/resolution/event_release.rs index c3022f9f4eaa..6053fd8c5eb0 100644 --- a/rust/cymbal/src/modes/processing/stages/event_release.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs @@ -2,20 +2,17 @@ use std::{collections::HashMap, sync::Arc, time::Duration}; use moka::future::{Cache, CacheBuilder}; use serde_json::Value; -use sqlx::{Executor, PgPool, Postgres}; +use sqlx::{Executor, Postgres}; use uuid::Uuid; use crate::{ - app_context::AppContext, error::UnhandledError, frames::releases::{mobile_release_hash_id, ReleaseRecord}, - metric_consts::{ANCILLARY_CACHE, EVENT_RELEASE_RESOLVER_OPERATOR, EVENT_RELEASE_STAGE}, - stages::pipeline::{HandledError, ParsedPipelineItem, ResolvedPipelineItem}, + metric_consts::{ANCILLARY_CACHE, EVENT_RELEASE_RESOLVER_OPERATOR}, + stages::{pipeline::HandledError, resolution::ResolutionStage}, types::{ - batch::Batch, exception_event::{ExceptionEvent, Parsed}, operator::{OperatorResult, TeamId, ValueOperator}, - stage::{Stage, StageResult}, }, }; @@ -30,7 +27,7 @@ use crate::{ /// content hash for mobile builds — so they get separate caches. `try_get_with` coalesces /// concurrent misses for the same key, so a cold cache under load issues one query per key rather /// than one per event. moka caches are internally Arc'd, so cloning this into each per-batch -/// `EventReleaseStage` is cheap. +/// `ResolutionStage` is cheap. /// /// Both caches are bounded in bytes rather than entries: a release carries a free-form `metadata` /// JSON column that any client can write, so entry count says nothing about memory held. @@ -122,43 +119,10 @@ fn record_cache_outcome(cache_type: &'static str, cache_miss: bool) { metrics::counter!(ANCILLARY_CACHE, "type" => cache_type, "outcome" => outcome).increment(1); } -/// Resolves each event's release and performs the Parsed -> Resolved flip. Runs after -/// `ResolutionStage`, which resolves frames in place and leaves events Parsed: release resolution -/// belongs on this side of the frame-resolution boundary so it can later fall back to the resolved -/// frames' symbol sets for legacy events that carry neither `$release_id` nor app metadata. -/// Owning `into_resolved()` here keeps `Resolved` born-complete, and means the compiler rejects a -/// pipeline that runs grouping before the release is known. -#[derive(Clone)] -pub struct EventReleaseStage { - pub posthog_pool: PgPool, - pub release_cache: ReleaseCache, -} - -impl From<&Arc> for EventReleaseStage { - fn from(ctx: &Arc) -> Self { - Self { - posthog_pool: ctx.posthog_pool.clone(), - release_cache: ctx.release_cache.clone(), - } - } -} - -impl Stage for EventReleaseStage { - type Input = ParsedPipelineItem; - type Output = ResolvedPipelineItem; - - fn name(&self) -> &'static str { - EVENT_RELEASE_STAGE - } - - async fn process(self, batch: Batch) -> StageResult { - let resolved = batch.apply_operator(EventReleaseResolver, self).await?; - Ok(resolved.map(|item, ()| item.map(|event| event.into_resolved()), &mut ())) - } -} - /// Resolves the event-level release without going through the per-frame symbol-set join, so the -/// release is independent of which chunks resolved the stack. +/// release is independent of which chunks resolved the stack. Runs inside `ResolutionStage` after +/// `resolve_batch`, so a future fallback for legacy events (which carry neither `$release_id` nor +/// app metadata) can read the resolved frames' symbol sets. /// /// Two sources, in order of preference: /// 1. `$release_id` — web builds inject the release row's id, which the SDK emits verbatim. Direct @@ -173,7 +137,7 @@ impl Stage for EventReleaseStage { pub struct EventReleaseResolver; impl ValueOperator for EventReleaseResolver { - type Context = EventReleaseStage; + type Context = ResolutionStage; type Item = ExceptionEvent; type HandledError = HandledError; type UnhandledError = UnhandledError; @@ -185,7 +149,7 @@ impl ValueOperator for EventReleaseResolver { async fn execute_value( &self, mut evt: ExceptionEvent, - ctx: EventReleaseStage, + ctx: ResolutionStage, ) -> OperatorResult { let release_id = evt .properties() diff --git a/rust/cymbal/src/modes/processing/stages/resolution/mod.rs b/rust/cymbal/src/modes/processing/stages/resolution/mod.rs index 2837ce3f7d74..054e99bf21a3 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/mod.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/mod.rs @@ -1,7 +1,9 @@ use std::sync::Arc; +use sqlx::PgPool; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; +pub mod event_release; pub mod exception; pub mod frame; pub mod remote; @@ -10,7 +12,8 @@ use crate::{ app_context::AppContext, error::UnhandledError, metric_consts::RESOLUTION_STAGE, - stages::pipeline::ParsedPipelineItem, + stages::pipeline::{ParsedPipelineItem, ResolvedPipelineItem}, + stages::resolution::event_release::{EventReleaseResolver, ReleaseCache}, stages::resolution::remote::resolver::{resolve_batch, RemoteResolutionContext}, symbolication::symbol::SymbolResolver, types::{ @@ -22,6 +25,8 @@ use crate::{ #[derive(Clone)] pub struct ResolutionStage { pub remote: RemoteResolutionContext, + pub posthog_pool: PgPool, + pub release_cache: ReleaseCache, } #[derive(Clone)] @@ -38,6 +43,8 @@ impl From<&Arc> for ResolutionStage { .remote_resolution .clone() .expect("processing app context requires remote resolution"), + posthog_pool: app_context.posthog_pool.clone(), + release_cache: app_context.release_cache.clone(), } } } @@ -56,15 +63,17 @@ impl LocalResolutionContext { impl Stage for ResolutionStage { type Input = ParsedPipelineItem; - // Frames are resolved in place and events stay Parsed; `EventReleaseStage` owns the - // Parsed -> Resolved flip so release resolution can read the resolved frames. - type Output = ParsedPipelineItem; + type Output = ResolvedPipelineItem; fn name(&self) -> &'static str { RESOLUTION_STAGE } async fn process(self, batch: Batch) -> StageResult { - resolve_batch(batch, self.remote).await + // Release resolution runs after resolve_batch so it can later fall back to the resolved + // frames' symbol sets for legacy events. + let resolved = resolve_batch(batch, self.remote.clone()).await?; + let resolved = resolved.apply_operator(EventReleaseResolver, self).await?; + Ok(resolved.map(|item, ()| item.map(|event| event.into_resolved()), &mut ())) } } diff --git a/rust/cymbal/tests/common/mod.rs b/rust/cymbal/tests/common/mod.rs index 4b858885a8ff..6fa923d4740d 100644 --- a/rust/cymbal/tests/common/mod.rs +++ b/rust/cymbal/tests/common/mod.rs @@ -21,6 +21,7 @@ use cymbal::frames::{Frame, RawFrame}; use cymbal::langs::native::DebugImage; use cymbal::stages::pipeline::ParsedPipelineItem; use cymbal::stages::resolution::{ + event_release::ReleaseCache, remote::{ config::RemoteResolutionConfig, pool::EndpointPool, resolver::RemoteResolutionContext, }, @@ -32,7 +33,7 @@ use cymbal::symbolication::symbol_store::proguard::ProguardRef; use cymbal::types::{ batch::Batch, event::AnyEvent, - exception_event::{ExceptionEvent, Parsed}, + exception_event::{ExceptionEvent, Parsed, Resolved}, operator::TeamId, stage::Stage, Exception, Stacktrace, @@ -424,15 +425,21 @@ impl SymbolResolver for NoopResolver { } pub fn remote_stage(ctx: RemoteResolutionContext) -> ResolutionStage { - ResolutionStage { remote: ctx } + ResolutionStage { + remote: ctx, + // Never connected: fixture events carry no release identifiers, so the release + // resolver never issues a query. + posthog_pool: sqlx::postgres::PgPoolOptions::new() + .connect_lazy("postgres://unused/unused") + .expect("lazy pool construction does not connect"), + release_cache: ReleaseCache::new(0, Duration::from_secs(0)), + } } -// The stage resolves frames in place and leaves events Parsed; the Parsed -> Resolved flip -// belongs to EventReleaseStage, outside these fixtures. pub async fn process_one( stage: ResolutionStage, evt: ExceptionEvent, -) -> Result, UnhandledError> { +) -> Result, UnhandledError> { let batch: Batch = Batch::from(vec![Ok(evt)]); let result = stage.process(batch).await?; let mut items: Vec<_> = result.into_iter().collect(); diff --git a/rust/cymbal/tests/remote_resolution.rs b/rust/cymbal/tests/remote_resolution.rs index 98aeec284aec..ddbab0ed5c5f 100644 --- a/rust/cymbal/tests/remote_resolution.rs +++ b/rust/cymbal/tests/remote_resolution.rs @@ -76,7 +76,7 @@ fn remote_stage_with_resolver( ctx: cymbal::stages::resolution::remote::resolver::RemoteResolutionContext, _resolver: Arc, ) -> ResolutionStage { - ResolutionStage { remote: ctx } + remote_stage(ctx) } fn parsed_event(uuid: Uuid, properties: serde_json::Value) -> ExceptionEvent { @@ -184,6 +184,13 @@ async fn happy_path_preserves_batch_event_and_exception_order() { }) .collect(); assert_eq!(resolved_types, expected_types); + for (resolved_evt, expected_evt_types) in resolved.iter().zip(expected_types.iter()) { + let mut expected_properties = expected_evt_types.clone(); + expected_properties.sort(); + let mut resolved_properties = resolved_evt.metadata().types.clone(); + resolved_properties.sort(); + assert_eq!(resolved_properties, expected_properties); + } } #[tokio::test] From d786796569b428acdd09b8bfab0fea65fd4fbeb7 Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Tue, 4 Aug 2026 12:36:33 +0200 Subject: [PATCH 6/9] feat(error-tracking): cap release metadata at 8 KB Cymbal embeds release metadata into every matching exception event, so an unbounded value stored once via the API would be amplified across the whole event stream, after capture's per-event size limit has already been enforced. - Release create/update reject metadata over 8 KB of serialized JSON - Cymbal clamps metadata at fetch for rows predating the cap, keeping the release id so the full row stays fetchable - With records bounded, the release cache drops its byte weigher for a plain entry-count budget (release_cache_max_bytes -> release_cache_max_entries) Co-Authored-By: Claude Fable 5 --- .../backend/presentation/views/releases.py | 26 +++++-- .../tests/api/test_error_tracking_api.py | 30 ++++++++ .../frontend/generated/api.schemas.ts | 12 ++-- .../frontend/generated/api.zod.ts | 8 ++- rust/cymbal/src/core/types/frames/releases.rs | 69 ++++++++----------- .../src/modes/processing/app_context.rs | 2 +- rust/cymbal/src/modes/processing/config.rs | 8 ++- .../stages/resolution/event_release.rs | 43 ++---------- services/mcp/src/api/generated.ts | 12 ++-- 9 files changed, 109 insertions(+), 101 deletions(-) diff --git a/products/error_tracking/backend/presentation/views/releases.py b/products/error_tracking/backend/presentation/views/releases.py index 7c25db922dfa..a90efba642d6 100644 --- a/products/error_tracking/backend/presentation/views/releases.py +++ b/products/error_tracking/backend/presentation/views/releases.py @@ -1,3 +1,5 @@ +import json + from drf_spectacular.utils import OpenApiResponse, extend_schema, extend_schema_field from rest_framework import serializers, status, viewsets from rest_framework.exceptions import NotFound, ValidationError @@ -14,6 +16,10 @@ from products.error_tracking.backend.presentation.pagination import paginate_via_facade MAX_HASH_ID_LENGTH = 128 +# Kept in sync with MAX_RELEASE_METADATA_BYTES in rust/cymbal/src/core/types/frames/releases.rs: +# cymbal embeds metadata into every matching exception event, so an unbounded value would be +# amplified across the whole event stream. +MAX_METADATA_BYTES = 8 * 1024 RELEASE_HASH_IN_USE_ERROR_CODE = "release_hash_in_use" @@ -37,7 +43,9 @@ class ErrorTrackingReleaseCreateRequestSerializer(serializers.Serializer): help_text="Optional client-supplied release hash (e.g. a git commit SHA). Generated server-side when omitted.", ) metadata = ReleaseMetadataField( - required=False, allow_null=True, help_text="Optional free-form metadata object stored alongside the release." + required=False, + allow_null=True, + help_text="Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON.", ) @@ -55,7 +63,9 @@ class ErrorTrackingReleaseUpdateRequestSerializer(serializers.Serializer): help_text="Release hash (e.g. a git commit SHA). Omit to preserve the current value.", ) metadata = ReleaseMetadataField( - required=False, allow_null=True, help_text="Free-form metadata object. Omit to preserve the current value." + required=False, + allow_null=True, + help_text="Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.", ) @@ -72,6 +82,13 @@ def _validated_hash_id(self, hash_id) -> str | None: raise ValidationError("Hash id length cannot exceed 128 bytes") return hash_id + def _validated_metadata(self, metadata: object) -> object: + if metadata is None: + return None + if len(json.dumps(metadata, separators=(",", ":")).encode("utf-8")) > MAX_METADATA_BYTES: + raise ValidationError("Metadata is too large. Keep it under 8 KB.") + return metadata + def list(self, request, *args, **kwargs) -> Response: return paginate_via_facade( self, @@ -97,13 +114,14 @@ def create(self, request, *args, **kwargs) -> Response: if not project: raise ValidationError("Project is required") hash_id = self._validated_hash_id(request.data.get("hash_id")) + metadata = self._validated_metadata(request.data.get("metadata")) try: release = error_tracking_api.create_release( self.team.id, version=str(version), project=str(project), hash_id=hash_id, - metadata=request.data.get("metadata"), + metadata=metadata, ) except error_tracking_api.ReleaseHashInUseError as err: raise ValidationError(f"Hash id {err} already in use", code=RELEASE_HASH_IN_USE_ERROR_CODE) from err @@ -115,7 +133,7 @@ def _apply_update(self, pk: str, data) -> Response: release = error_tracking_api.update_release( self.team.id, pk, - metadata=data.get("metadata"), + metadata=self._validated_metadata(data.get("metadata")), hash_id=hash_id, version=data.get("version"), project=data.get("project"), diff --git a/products/error_tracking/backend/tests/api/test_error_tracking_api.py b/products/error_tracking/backend/tests/api/test_error_tracking_api.py index 2175f25b9f69..73f834754d49 100644 --- a/products/error_tracking/backend/tests/api/test_error_tracking_api.py +++ b/products/error_tracking/backend/tests/api/test_error_tracking_api.py @@ -1277,6 +1277,36 @@ def test_releases_list_paginates_in_sql(self) -> None: ] assert limited_selects, "expected a LIMIT 2 SELECT on the release table" + @parameterized.expand(["create", "update"]) + def test_release_rejects_oversized_metadata(self, method: str) -> None: + # Cymbal embeds release metadata into every matching exception event, so an uncapped + # value would be amplified across the whole event stream. + oversized = {"blob": "x" * (8 * 1024)} + if method == "create": + response = self.client.post( + f"/api/environments/{self.team.id}/error_tracking/releases", + data={"version": "1.0.0", "project": "proj", "metadata": oversized}, + format="json", + ) + else: + release = ErrorTrackingRelease.objects.create(team=self.team, hash_id="h", version="1.0.0", project="proj") + response = self.client.patch( + f"/api/environments/{self.team.id}/error_tracking/releases/{release.id}", + data={"metadata": oversized}, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Metadata is too large" in response.json()["detail"] + + def test_release_accepts_small_metadata(self) -> None: + response = self.client.post( + f"/api/environments/{self.team.id}/error_tracking/releases", + data={"version": "1.0.0", "project": "proj", "metadata": {"git": {"commit_id": "abc123"}}}, + format="json", + ) + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["metadata"] == {"git": {"commit_id": "abc123"}} + class TestIssueStateSync(ClickhouseTestMixin, APIBaseTest): def _create_issue(self, fingerprints=None, **kwargs) -> ErrorTrackingIssue: diff --git a/products/error_tracking/frontend/generated/api.schemas.ts b/products/error_tracking/frontend/generated/api.schemas.ts index 255ff2210723..3e8f24ad34b0 100644 --- a/products/error_tracking/frontend/generated/api.schemas.ts +++ b/products/error_tracking/frontend/generated/api.schemas.ts @@ -1389,7 +1389,7 @@ export interface PaginatedErrorTrackingReleaseListApi { } /** - * Optional free-form metadata object stored alongside the release. + * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. * @nullable */ export type ErrorTrackingReleaseCreateRequestApiMetadata = { [key: string]: unknown } | null @@ -1406,14 +1406,14 @@ export interface ErrorTrackingReleaseCreateRequestApi { */ hash_id?: string | null /** - * Optional free-form metadata object stored alongside the release. + * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. * @nullable */ metadata?: ErrorTrackingReleaseCreateRequestApiMetadata } /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ export type ErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string]: unknown } | null @@ -1436,14 +1436,14 @@ export interface ErrorTrackingReleaseUpdateRequestApi { */ hash_id?: string | null /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ metadata?: ErrorTrackingReleaseUpdateRequestApiMetadata } /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ export type PatchedErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string]: unknown } | null @@ -1466,7 +1466,7 @@ export interface PatchedErrorTrackingReleaseUpdateRequestApi { */ hash_id?: string | null /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ metadata?: PatchedErrorTrackingReleaseUpdateRequestApiMetadata diff --git a/products/error_tracking/frontend/generated/api.zod.ts b/products/error_tracking/frontend/generated/api.zod.ts index 2ade6536c597..0f14fc278f95 100644 --- a/products/error_tracking/frontend/generated/api.zod.ts +++ b/products/error_tracking/frontend/generated/api.zod.ts @@ -894,7 +894,9 @@ export const ErrorTrackingReleasesCreateBody = /* @__PURE__ */ zod.object({ metadata: zod .record(zod.string(), zod.unknown()) .nullish() - .describe('Optional free-form metadata object stored alongside the release.'), + .describe( + 'Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON.' + ), }) export const errorTrackingReleasesUpdateBodyHashIdMax = 128 @@ -910,7 +912,7 @@ export const ErrorTrackingReleasesUpdateBody = /* @__PURE__ */ zod.object({ metadata: zod .record(zod.string(), zod.unknown()) .nullish() - .describe('Free-form metadata object. Omit to preserve the current value.'), + .describe('Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.'), }) export const errorTrackingReleasesPartialUpdateBodyHashIdMax = 128 @@ -926,7 +928,7 @@ export const ErrorTrackingReleasesPartialUpdateBody = /* @__PURE__ */ zod.object metadata: zod .record(zod.string(), zod.unknown()) .nullish() - .describe('Free-form metadata object. Omit to preserve the current value.'), + .describe('Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.'), }) export const ErrorTrackingSettingsUpdateSettingsPartialUpdateBody = /* @__PURE__ */ zod.object({ diff --git a/rust/cymbal/src/core/types/frames/releases.rs b/rust/cymbal/src/core/types/frames/releases.rs index 07181ba5a83f..9311c3b2f4a6 100644 --- a/rust/cymbal/src/core/types/frames/releases.rs +++ b/rust/cymbal/src/core/types/frames/releases.rs @@ -5,6 +5,13 @@ use sha2::{Digest, Sha512}; use sqlx::Executor; use uuid::Uuid; +/// Kept in sync with MAX_METADATA_BYTES in the release API +/// (products/error_tracking/backend/presentation/views/releases.py). The API rejects larger +/// metadata on write; rows predating the cap (or written outside it) are clamped at fetch so a +/// single oversized release can't be amplified into every matching event, and so cache entries +/// stay small enough for an entry-count budget. +pub const MAX_RELEASE_METADATA_BYTES: usize = 8 * 1024; + #[derive(Debug, Clone, Eq, PartialEq)] pub struct ReleaseRecord { pub id: Uuid, @@ -47,7 +54,7 @@ impl ReleaseRecord { .fetch_optional(e) .await?; - Ok(row) + Ok(row.map(Self::with_clamped_metadata)) } pub async fn for_hash<'c, E>( @@ -71,7 +78,7 @@ impl ReleaseRecord { .fetch_optional(e) .await?; - Ok(row) + Ok(row.map(Self::with_clamped_metadata)) } pub fn to_info(&self) -> ReleaseInfo { @@ -84,35 +91,16 @@ impl ReleaseRecord { } } - /// Rough in-memory footprint, for the release cache's weigher. `metadata` is a free-form - /// JSON column any client can write, so it dominates and is the only reason this exists — - /// without it a cache bounded on entry count would be unbounded in bytes. Only has to be - /// proportional to the real cost, not exact. - pub fn approx_size_bytes(&self) -> usize { - size_of::() - + self.hash_id.len() - + self.version.len() - + self.project.len() - + self.metadata.as_ref().map_or(0, json_size_bytes) - } -} - -/// Heap bytes held by a `Value`, ignoring the inline scalars already counted by `size_of`. -/// -/// The recursion is bounded: these values are decoded by `serde_json`, which enforces its own -/// nesting limit while parsing, so a hostile `metadata` column can't drive this deep enough to -/// overflow the stack. -fn json_size_bytes(value: &Value) -> usize { - match value { - Value::Null | Value::Bool(_) | Value::Number(_) => 0, - Value::String(s) => s.len(), - Value::Array(items) => { - items.len() * size_of::() + items.iter().map(json_size_bytes).sum::() + /// Drops `metadata` when its serialized form exceeds the cap the API enforces on new writes. + /// The `id` survives, so consumers can still fetch the full release. + fn with_clamped_metadata(mut self) -> Self { + let oversized = self.metadata.as_ref().is_some_and(|metadata| { + serde_json::to_string(metadata).map_or(true, |s| s.len() > MAX_RELEASE_METADATA_BYTES) + }); + if oversized { + self.metadata = None; } - Value::Object(entries) => entries - .iter() - .map(|(key, val)| key.len() + size_of::() + json_size_bytes(val)) - .sum(), + self } } @@ -211,17 +199,14 @@ mod tests { } #[test] - fn size_estimate_tracks_metadata_payload() { - // The cache weigher is only a real memory bound if the estimate actually grows with the - // free-form `metadata` column. Returning a constant here (or ignoring nested strings) - // would silently restore the unbounded-by-entry-count behavior the weigher replaced. - let blob = "x".repeat(100_000); - let bare = record(None).approx_size_bytes(); - let nested = record(Some(json!({"git": {"commit_id": blob}}))).approx_size_bytes(); - - assert!( - nested >= bare + 100_000, - "nested metadata under-counted: {nested} vs {bare}" - ); + fn oversized_metadata_is_clamped_but_small_metadata_survives() { + // Rows predating the API's write-time cap can hold multi-megabyte metadata; without the + // clamp every matching event would embed it, re-opening the amplification the cap closed. + let big = record(Some(json!({"git": {"commit_id": "x".repeat(100_000)}}))); + assert_eq!(big.with_clamped_metadata().metadata, None); + + let small_value = json!({"git": {"commit_id": "abc123"}}); + let small = record(Some(small_value.clone())); + assert_eq!(small.with_clamped_metadata().metadata, Some(small_value)); } } diff --git a/rust/cymbal/src/modes/processing/app_context.rs b/rust/cymbal/src/modes/processing/app_context.rs index 028366fc0ed9..2caa87c82676 100644 --- a/rust/cymbal/src/modes/processing/app_context.rs +++ b/rust/cymbal/src/modes/processing/app_context.rs @@ -167,7 +167,7 @@ impl AppContext { .build(); let release_cache = ReleaseCache::new( - config.release_cache_max_bytes, + config.release_cache_max_entries, Duration::from_secs(config.release_cache_ttl_seconds), ); diff --git a/rust/cymbal/src/modes/processing/config.rs b/rust/cymbal/src/modes/processing/config.rs index e39d90fe0ed3..2332cc4c21fe 100644 --- a/rust/cymbal/src/modes/processing/config.rs +++ b/rust/cymbal/src/modes/processing/config.rs @@ -86,9 +86,11 @@ pub struct ProcessingConfig { #[envconfig(default = "300")] pub release_cache_ttl_seconds: u64, - // Bounded in bytes in case someone tries to do something funny. - #[envconfig(default = "33554432")] // 32 MiB - pub release_cache_max_bytes: u64, + // An entry-count bound is a real memory bound: cached records clamp metadata to + // MAX_RELEASE_METADATA_BYTES at fetch, so a full cache tops out around + // max_entries * 8 KiB per lookup kind. + #[envconfig(default = "10000")] + pub release_cache_max_entries: u64, // Maximum number of in-flight futures for a single `Batch::apply_func` call. // This is a per-call-site limit, not a global pipeline-wide concurrency cap. diff --git a/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs index 6053fd8c5eb0..6a43756a7eb2 100644 --- a/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs +++ b/rust/cymbal/src/modes/processing/stages/resolution/event_release.rs @@ -29,41 +29,21 @@ use crate::{ /// than one per event. moka caches are internally Arc'd, so cloning this into each per-batch /// `ResolutionStage` is cheap. /// -/// Both caches are bounded in bytes rather than entries: a release carries a free-form `metadata` -/// JSON column that any client can write, so entry count says nothing about memory held. +/// An entry-count budget is a real memory bound here because every cached record is small: +/// `metadata` is clamped to `MAX_RELEASE_METADATA_BYTES` at fetch, and negative entries (`None`) +/// are near-empty. #[derive(Clone)] pub struct ReleaseCache { by_id: Cache<(TeamId, Uuid), Option>, by_hash: Cache<(TeamId, String), Option>, } -/// Charged on top of the payload for every entry, covering the key and moka's own per-entry -/// bookkeeping. It also keeps a negative entry (`None`) from weighing nothing: misses are the -/// high-cardinality side — one per app that never bound a release — so weightless negatives would -/// let the cache grow without bound, which is the whole thing the byte budget exists to stop. -const CACHE_ENTRY_OVERHEAD_BYTES: usize = 128; - -fn entry_weight(key_bytes: usize, value: &Option) -> u32 { - let bytes = CACHE_ENTRY_OVERHEAD_BYTES - + key_bytes - + value.as_ref().map_or(0, ReleaseRecord::approx_size_bytes); - bytes.try_into().unwrap_or(u32::MAX) -} - impl ReleaseCache { - /// `max_bytes` bounds each of the two caches independently, so the pair can hold twice that. - pub fn new(max_bytes: u64, ttl: Duration) -> Self { + /// `max_entries` bounds each of the two caches independently, so the pair can hold twice that. + pub fn new(max_entries: u64, ttl: Duration) -> Self { Self { - by_id: CacheBuilder::new(max_bytes) - .weigher(|_key, value: &Option| entry_weight(0, value)) - .time_to_live(ttl) - .build(), - by_hash: CacheBuilder::new(max_bytes) - .weigher(|key: &(TeamId, String), value: &Option| { - entry_weight(key.1.len(), value) - }) - .time_to_live(ttl) - .build(), + by_id: CacheBuilder::new(max_entries).time_to_live(ttl).build(), + by_hash: CacheBuilder::new(max_entries).time_to_live(ttl).build(), } } @@ -209,15 +189,6 @@ mod tests { .collect() } - #[test] - fn negative_entries_are_not_weightless() { - // Misses are the high-cardinality side of this cache — one per app that never bound a - // release — so a zero-weight `None` would let the by-hash cache grow without bound under - // the byte budget, which is exactly what the weigher exists to prevent. - assert!(entry_weight(0, &None) > 0); - assert!(entry_weight(128, &None) > entry_weight(0, &None)); - } - #[test] fn numeric_and_string_build_hash_identically() { // The iOS SDK parses a numeric CFBundleVersion into an Int, so `$app_build` arrives as a JSON diff --git a/services/mcp/src/api/generated.ts b/services/mcp/src/api/generated.ts index a5b1458c4fdc..c948d729f082 100644 --- a/services/mcp/src/api/generated.ts +++ b/services/mcp/src/api/generated.ts @@ -26574,7 +26574,7 @@ export namespace Schemas { } /** - * Optional free-form metadata object stored alongside the release. + * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. * @nullable */ export type ErrorTrackingReleaseCreateRequestMetadata = { [key: string]: unknown } | null; @@ -26591,14 +26591,14 @@ export namespace Schemas { */ hash_id?: string | null; /** - * Optional free-form metadata object stored alongside the release. + * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. * @nullable */ metadata?: ErrorTrackingReleaseCreateRequestMetadata; } /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ export type ErrorTrackingReleaseUpdateRequestMetadata = { [key: string]: unknown } | null; @@ -26621,7 +26621,7 @@ export namespace Schemas { */ hash_id?: string | null; /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ metadata?: ErrorTrackingReleaseUpdateRequestMetadata; @@ -51654,7 +51654,7 @@ export namespace Schemas { } /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ export type PatchedErrorTrackingReleaseUpdateRequestMetadata = { [key: string]: unknown } | null; @@ -51677,7 +51677,7 @@ export namespace Schemas { */ hash_id?: string | null; /** - * Free-form metadata object. Omit to preserve the current value. + * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. * @nullable */ metadata?: PatchedErrorTrackingReleaseUpdateRequestMetadata; From 9117e663164d5560f25407c1b846a259168a7c68 Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Tue, 4 Aug 2026 12:48:11 +0200 Subject: [PATCH 7/9] feat(error-tracking): cap release version and project length Same amplification path as metadata: both strings are embedded into every matching exception event, and neither needs more room than a long semver, commit SHA, or bundle identifier. Create/update reject values over 255 characters; the limit is declared on the request serializers so generated types carry it. Co-Authored-By: Claude Fable 5 --- .../backend/presentation/views/releases.py | 42 +++++++++++++---- .../tests/api/test_error_tracking_api.py | 13 ++++++ .../frontend/generated/api.schemas.ts | 14 +++++- .../frontend/generated/api.zod.ts | 46 ++++++++++++++++--- services/mcp/src/api/generated.ts | 14 +++++- 5 files changed, 109 insertions(+), 20 deletions(-) diff --git a/products/error_tracking/backend/presentation/views/releases.py b/products/error_tracking/backend/presentation/views/releases.py index a90efba642d6..e688b83e5a99 100644 --- a/products/error_tracking/backend/presentation/views/releases.py +++ b/products/error_tracking/backend/presentation/views/releases.py @@ -16,6 +16,9 @@ from products.error_tracking.backend.presentation.pagination import paginate_via_facade MAX_HASH_ID_LENGTH = 128 +# Version and project are embedded into every matching exception event by cymbal, and neither +# needs more room than a long semver, commit SHA, or bundle identifier. +MAX_TEXT_LENGTH = 255 # Kept in sync with MAX_RELEASE_METADATA_BYTES in rust/cymbal/src/core/types/frames/releases.rs: # cymbal embeds metadata into every matching exception event, so an unbounded value would be # amplified across the whole event stream. @@ -34,8 +37,13 @@ class Meta: class ErrorTrackingReleaseCreateRequestSerializer(serializers.Serializer): - version = serializers.CharField(help_text="Human-readable release version, e.g. a semver string or build number.") - project = serializers.CharField(help_text="Identifier of the project this release belongs to.") + version = serializers.CharField( + max_length=MAX_TEXT_LENGTH, + help_text="Human-readable release version, e.g. a semver string or build number.", + ) + project = serializers.CharField( + max_length=MAX_TEXT_LENGTH, help_text="Identifier of the project this release belongs to." + ) hash_id = serializers.CharField( required=False, allow_null=True, @@ -51,10 +59,16 @@ class ErrorTrackingReleaseCreateRequestSerializer(serializers.Serializer): class ErrorTrackingReleaseUpdateRequestSerializer(serializers.Serializer): version = serializers.CharField( - required=False, allow_null=True, help_text="Human-readable release version. Omit to preserve the current value." + required=False, + allow_null=True, + max_length=MAX_TEXT_LENGTH, + help_text="Human-readable release version. Omit to preserve the current value.", ) project = serializers.CharField( - required=False, allow_null=True, help_text="Project identifier. Omit to preserve the current value." + required=False, + allow_null=True, + max_length=MAX_TEXT_LENGTH, + help_text="Project identifier. Omit to preserve the current value.", ) hash_id = serializers.CharField( required=False, @@ -89,6 +103,14 @@ def _validated_metadata(self, metadata: object) -> object: raise ValidationError("Metadata is too large. Keep it under 8 KB.") return metadata + def _validated_text(self, value: object, field: str) -> str | None: + if value is None: + return None + value = str(value) + if len(value) > MAX_TEXT_LENGTH: + raise ValidationError(f"{field} is too long. Keep it under {MAX_TEXT_LENGTH} characters.") + return value + def list(self, request, *args, **kwargs) -> Response: return paginate_via_facade( self, @@ -107,8 +129,8 @@ def retrieve(self, request, *args, pk=None, **kwargs) -> Response: responses={201: OpenApiResponse(response=ErrorTrackingReleaseSerializer)}, ) def create(self, request, *args, **kwargs) -> Response: - version = request.data.get("version") - project = request.data.get("project") + version = self._validated_text(request.data.get("version"), "Version") + project = self._validated_text(request.data.get("project"), "Project") if not version: raise ValidationError("Version is required") if not project: @@ -118,8 +140,8 @@ def create(self, request, *args, **kwargs) -> Response: try: release = error_tracking_api.create_release( self.team.id, - version=str(version), - project=str(project), + version=version, + project=project, hash_id=hash_id, metadata=metadata, ) @@ -135,8 +157,8 @@ def _apply_update(self, pk: str, data) -> Response: pk, metadata=self._validated_metadata(data.get("metadata")), hash_id=hash_id, - version=data.get("version"), - project=data.get("project"), + version=self._validated_text(data.get("version"), "Version"), + project=self._validated_text(data.get("project"), "Project"), ) except error_tracking_api.ReleaseHashInUseError as err: raise ValidationError(f"Hash id {err} already in use", code=RELEASE_HASH_IN_USE_ERROR_CODE) from err diff --git a/products/error_tracking/backend/tests/api/test_error_tracking_api.py b/products/error_tracking/backend/tests/api/test_error_tracking_api.py index 73f834754d49..39e52f5319c1 100644 --- a/products/error_tracking/backend/tests/api/test_error_tracking_api.py +++ b/products/error_tracking/backend/tests/api/test_error_tracking_api.py @@ -1298,6 +1298,19 @@ def test_release_rejects_oversized_metadata(self, method: str) -> None: assert response.status_code == status.HTTP_400_BAD_REQUEST assert "Metadata is too large" in response.json()["detail"] + @parameterized.expand(["version", "project"]) + def test_release_rejects_overlong_text_fields(self, field: str) -> None: + # Version and project are embedded into every matching exception event, so they carry + # the same amplification risk as metadata if left uncapped. + data = {"version": "1.0.0", "project": "proj", field: "x" * 256} + response = self.client.post( + f"/api/environments/{self.team.id}/error_tracking/releases", + data=data, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "too long" in response.json()["detail"] + def test_release_accepts_small_metadata(self) -> None: response = self.client.post( f"/api/environments/{self.team.id}/error_tracking/releases", diff --git a/products/error_tracking/frontend/generated/api.schemas.ts b/products/error_tracking/frontend/generated/api.schemas.ts index 3e8f24ad34b0..fed8665c25a0 100644 --- a/products/error_tracking/frontend/generated/api.schemas.ts +++ b/products/error_tracking/frontend/generated/api.schemas.ts @@ -1395,9 +1395,15 @@ export interface PaginatedErrorTrackingReleaseListApi { export type ErrorTrackingReleaseCreateRequestApiMetadata = { [key: string]: unknown } | null export interface ErrorTrackingReleaseCreateRequestApi { - /** Human-readable release version, e.g. a semver string or build number. */ + /** + * Human-readable release version, e.g. a semver string or build number. + * @maxLength 255 + */ version: string - /** Identifier of the project this release belongs to. */ + /** + * Identifier of the project this release belongs to. + * @maxLength 255 + */ project: string /** * Optional client-supplied release hash (e.g. a git commit SHA). Generated server-side when omitted. @@ -1421,11 +1427,13 @@ export type ErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string]: unkn export interface ErrorTrackingReleaseUpdateRequestApi { /** * Human-readable release version. Omit to preserve the current value. + * @maxLength 255 * @nullable */ version?: string | null /** * Project identifier. Omit to preserve the current value. + * @maxLength 255 * @nullable */ project?: string | null @@ -1451,11 +1459,13 @@ export type PatchedErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string export interface PatchedErrorTrackingReleaseUpdateRequestApi { /** * Human-readable release version. Omit to preserve the current value. + * @maxLength 255 * @nullable */ version?: string | null /** * Project identifier. Omit to preserve the current value. + * @maxLength 255 * @nullable */ project?: string | null diff --git a/products/error_tracking/frontend/generated/api.zod.ts b/products/error_tracking/frontend/generated/api.zod.ts index 0f14fc278f95..115be85ba885 100644 --- a/products/error_tracking/frontend/generated/api.zod.ts +++ b/products/error_tracking/frontend/generated/api.zod.ts @@ -881,11 +881,21 @@ export const ErrorTrackingQueryIssuesListCreateBody = /* @__PURE__ */ zod.object .describe('Search stack-frame source\/file path text.'), }) +export const errorTrackingReleasesCreateBodyVersionMax = 255 + +export const errorTrackingReleasesCreateBodyProjectMax = 255 + export const errorTrackingReleasesCreateBodyHashIdMax = 128 export const ErrorTrackingReleasesCreateBody = /* @__PURE__ */ zod.object({ - version: zod.string().describe('Human-readable release version, e.g. a semver string or build number.'), - project: zod.string().describe('Identifier of the project this release belongs to.'), + version: zod + .string() + .max(errorTrackingReleasesCreateBodyVersionMax) + .describe('Human-readable release version, e.g. a semver string or build number.'), + project: zod + .string() + .max(errorTrackingReleasesCreateBodyProjectMax) + .describe('Identifier of the project this release belongs to.'), hash_id: zod .string() .max(errorTrackingReleasesCreateBodyHashIdMax) @@ -899,11 +909,23 @@ export const ErrorTrackingReleasesCreateBody = /* @__PURE__ */ zod.object({ ), }) +export const errorTrackingReleasesUpdateBodyVersionMax = 255 + +export const errorTrackingReleasesUpdateBodyProjectMax = 255 + export const errorTrackingReleasesUpdateBodyHashIdMax = 128 export const ErrorTrackingReleasesUpdateBody = /* @__PURE__ */ zod.object({ - version: zod.string().nullish().describe('Human-readable release version. Omit to preserve the current value.'), - project: zod.string().nullish().describe('Project identifier. Omit to preserve the current value.'), + version: zod + .string() + .max(errorTrackingReleasesUpdateBodyVersionMax) + .nullish() + .describe('Human-readable release version. Omit to preserve the current value.'), + project: zod + .string() + .max(errorTrackingReleasesUpdateBodyProjectMax) + .nullish() + .describe('Project identifier. Omit to preserve the current value.'), hash_id: zod .string() .max(errorTrackingReleasesUpdateBodyHashIdMax) @@ -915,11 +937,23 @@ export const ErrorTrackingReleasesUpdateBody = /* @__PURE__ */ zod.object({ .describe('Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.'), }) +export const errorTrackingReleasesPartialUpdateBodyVersionMax = 255 + +export const errorTrackingReleasesPartialUpdateBodyProjectMax = 255 + export const errorTrackingReleasesPartialUpdateBodyHashIdMax = 128 export const ErrorTrackingReleasesPartialUpdateBody = /* @__PURE__ */ zod.object({ - version: zod.string().nullish().describe('Human-readable release version. Omit to preserve the current value.'), - project: zod.string().nullish().describe('Project identifier. Omit to preserve the current value.'), + version: zod + .string() + .max(errorTrackingReleasesPartialUpdateBodyVersionMax) + .nullish() + .describe('Human-readable release version. Omit to preserve the current value.'), + project: zod + .string() + .max(errorTrackingReleasesPartialUpdateBodyProjectMax) + .nullish() + .describe('Project identifier. Omit to preserve the current value.'), hash_id: zod .string() .max(errorTrackingReleasesPartialUpdateBodyHashIdMax) diff --git a/services/mcp/src/api/generated.ts b/services/mcp/src/api/generated.ts index c948d729f082..e9fc8e4fe61b 100644 --- a/services/mcp/src/api/generated.ts +++ b/services/mcp/src/api/generated.ts @@ -26580,9 +26580,15 @@ export namespace Schemas { export type ErrorTrackingReleaseCreateRequestMetadata = { [key: string]: unknown } | null; export interface ErrorTrackingReleaseCreateRequest { - /** Human-readable release version, e.g. a semver string or build number. */ + /** + * Human-readable release version, e.g. a semver string or build number. + * @maxLength 255 + */ version: string; - /** Identifier of the project this release belongs to. */ + /** + * Identifier of the project this release belongs to. + * @maxLength 255 + */ project: string; /** * Optional client-supplied release hash (e.g. a git commit SHA). Generated server-side when omitted. @@ -26606,11 +26612,13 @@ export namespace Schemas { export interface ErrorTrackingReleaseUpdateRequest { /** * Human-readable release version. Omit to preserve the current value. + * @maxLength 255 * @nullable */ version?: string | null; /** * Project identifier. Omit to preserve the current value. + * @maxLength 255 * @nullable */ project?: string | null; @@ -51662,11 +51670,13 @@ export namespace Schemas { export interface PatchedErrorTrackingReleaseUpdateRequest { /** * Human-readable release version. Omit to preserve the current value. + * @maxLength 255 * @nullable */ version?: string | null; /** * Project identifier. Omit to preserve the current value. + * @maxLength 255 * @nullable */ project?: string | null; From ae748100ad69166aac0f93ed02a95cbc915e9a57 Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Tue, 4 Aug 2026 13:04:26 +0200 Subject: [PATCH 8/9] Revert "feat(error-tracking): cap release version and project length" This reverts commit 9117e663164d5560f25407c1b846a259168a7c68. --- .../backend/presentation/views/releases.py | 42 ++++------------- .../tests/api/test_error_tracking_api.py | 13 ------ .../frontend/generated/api.schemas.ts | 14 +----- .../frontend/generated/api.zod.ts | 46 +++---------------- services/mcp/src/api/generated.ts | 14 +----- 5 files changed, 20 insertions(+), 109 deletions(-) diff --git a/products/error_tracking/backend/presentation/views/releases.py b/products/error_tracking/backend/presentation/views/releases.py index e688b83e5a99..a90efba642d6 100644 --- a/products/error_tracking/backend/presentation/views/releases.py +++ b/products/error_tracking/backend/presentation/views/releases.py @@ -16,9 +16,6 @@ from products.error_tracking.backend.presentation.pagination import paginate_via_facade MAX_HASH_ID_LENGTH = 128 -# Version and project are embedded into every matching exception event by cymbal, and neither -# needs more room than a long semver, commit SHA, or bundle identifier. -MAX_TEXT_LENGTH = 255 # Kept in sync with MAX_RELEASE_METADATA_BYTES in rust/cymbal/src/core/types/frames/releases.rs: # cymbal embeds metadata into every matching exception event, so an unbounded value would be # amplified across the whole event stream. @@ -37,13 +34,8 @@ class Meta: class ErrorTrackingReleaseCreateRequestSerializer(serializers.Serializer): - version = serializers.CharField( - max_length=MAX_TEXT_LENGTH, - help_text="Human-readable release version, e.g. a semver string or build number.", - ) - project = serializers.CharField( - max_length=MAX_TEXT_LENGTH, help_text="Identifier of the project this release belongs to." - ) + version = serializers.CharField(help_text="Human-readable release version, e.g. a semver string or build number.") + project = serializers.CharField(help_text="Identifier of the project this release belongs to.") hash_id = serializers.CharField( required=False, allow_null=True, @@ -59,16 +51,10 @@ class ErrorTrackingReleaseCreateRequestSerializer(serializers.Serializer): class ErrorTrackingReleaseUpdateRequestSerializer(serializers.Serializer): version = serializers.CharField( - required=False, - allow_null=True, - max_length=MAX_TEXT_LENGTH, - help_text="Human-readable release version. Omit to preserve the current value.", + required=False, allow_null=True, help_text="Human-readable release version. Omit to preserve the current value." ) project = serializers.CharField( - required=False, - allow_null=True, - max_length=MAX_TEXT_LENGTH, - help_text="Project identifier. Omit to preserve the current value.", + required=False, allow_null=True, help_text="Project identifier. Omit to preserve the current value." ) hash_id = serializers.CharField( required=False, @@ -103,14 +89,6 @@ def _validated_metadata(self, metadata: object) -> object: raise ValidationError("Metadata is too large. Keep it under 8 KB.") return metadata - def _validated_text(self, value: object, field: str) -> str | None: - if value is None: - return None - value = str(value) - if len(value) > MAX_TEXT_LENGTH: - raise ValidationError(f"{field} is too long. Keep it under {MAX_TEXT_LENGTH} characters.") - return value - def list(self, request, *args, **kwargs) -> Response: return paginate_via_facade( self, @@ -129,8 +107,8 @@ def retrieve(self, request, *args, pk=None, **kwargs) -> Response: responses={201: OpenApiResponse(response=ErrorTrackingReleaseSerializer)}, ) def create(self, request, *args, **kwargs) -> Response: - version = self._validated_text(request.data.get("version"), "Version") - project = self._validated_text(request.data.get("project"), "Project") + version = request.data.get("version") + project = request.data.get("project") if not version: raise ValidationError("Version is required") if not project: @@ -140,8 +118,8 @@ def create(self, request, *args, **kwargs) -> Response: try: release = error_tracking_api.create_release( self.team.id, - version=version, - project=project, + version=str(version), + project=str(project), hash_id=hash_id, metadata=metadata, ) @@ -157,8 +135,8 @@ def _apply_update(self, pk: str, data) -> Response: pk, metadata=self._validated_metadata(data.get("metadata")), hash_id=hash_id, - version=self._validated_text(data.get("version"), "Version"), - project=self._validated_text(data.get("project"), "Project"), + version=data.get("version"), + project=data.get("project"), ) except error_tracking_api.ReleaseHashInUseError as err: raise ValidationError(f"Hash id {err} already in use", code=RELEASE_HASH_IN_USE_ERROR_CODE) from err diff --git a/products/error_tracking/backend/tests/api/test_error_tracking_api.py b/products/error_tracking/backend/tests/api/test_error_tracking_api.py index 39e52f5319c1..73f834754d49 100644 --- a/products/error_tracking/backend/tests/api/test_error_tracking_api.py +++ b/products/error_tracking/backend/tests/api/test_error_tracking_api.py @@ -1298,19 +1298,6 @@ def test_release_rejects_oversized_metadata(self, method: str) -> None: assert response.status_code == status.HTTP_400_BAD_REQUEST assert "Metadata is too large" in response.json()["detail"] - @parameterized.expand(["version", "project"]) - def test_release_rejects_overlong_text_fields(self, field: str) -> None: - # Version and project are embedded into every matching exception event, so they carry - # the same amplification risk as metadata if left uncapped. - data = {"version": "1.0.0", "project": "proj", field: "x" * 256} - response = self.client.post( - f"/api/environments/{self.team.id}/error_tracking/releases", - data=data, - format="json", - ) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "too long" in response.json()["detail"] - def test_release_accepts_small_metadata(self) -> None: response = self.client.post( f"/api/environments/{self.team.id}/error_tracking/releases", diff --git a/products/error_tracking/frontend/generated/api.schemas.ts b/products/error_tracking/frontend/generated/api.schemas.ts index fed8665c25a0..3e8f24ad34b0 100644 --- a/products/error_tracking/frontend/generated/api.schemas.ts +++ b/products/error_tracking/frontend/generated/api.schemas.ts @@ -1395,15 +1395,9 @@ export interface PaginatedErrorTrackingReleaseListApi { export type ErrorTrackingReleaseCreateRequestApiMetadata = { [key: string]: unknown } | null export interface ErrorTrackingReleaseCreateRequestApi { - /** - * Human-readable release version, e.g. a semver string or build number. - * @maxLength 255 - */ + /** Human-readable release version, e.g. a semver string or build number. */ version: string - /** - * Identifier of the project this release belongs to. - * @maxLength 255 - */ + /** Identifier of the project this release belongs to. */ project: string /** * Optional client-supplied release hash (e.g. a git commit SHA). Generated server-side when omitted. @@ -1427,13 +1421,11 @@ export type ErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string]: unkn export interface ErrorTrackingReleaseUpdateRequestApi { /** * Human-readable release version. Omit to preserve the current value. - * @maxLength 255 * @nullable */ version?: string | null /** * Project identifier. Omit to preserve the current value. - * @maxLength 255 * @nullable */ project?: string | null @@ -1459,13 +1451,11 @@ export type PatchedErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string export interface PatchedErrorTrackingReleaseUpdateRequestApi { /** * Human-readable release version. Omit to preserve the current value. - * @maxLength 255 * @nullable */ version?: string | null /** * Project identifier. Omit to preserve the current value. - * @maxLength 255 * @nullable */ project?: string | null diff --git a/products/error_tracking/frontend/generated/api.zod.ts b/products/error_tracking/frontend/generated/api.zod.ts index 115be85ba885..0f14fc278f95 100644 --- a/products/error_tracking/frontend/generated/api.zod.ts +++ b/products/error_tracking/frontend/generated/api.zod.ts @@ -881,21 +881,11 @@ export const ErrorTrackingQueryIssuesListCreateBody = /* @__PURE__ */ zod.object .describe('Search stack-frame source\/file path text.'), }) -export const errorTrackingReleasesCreateBodyVersionMax = 255 - -export const errorTrackingReleasesCreateBodyProjectMax = 255 - export const errorTrackingReleasesCreateBodyHashIdMax = 128 export const ErrorTrackingReleasesCreateBody = /* @__PURE__ */ zod.object({ - version: zod - .string() - .max(errorTrackingReleasesCreateBodyVersionMax) - .describe('Human-readable release version, e.g. a semver string or build number.'), - project: zod - .string() - .max(errorTrackingReleasesCreateBodyProjectMax) - .describe('Identifier of the project this release belongs to.'), + version: zod.string().describe('Human-readable release version, e.g. a semver string or build number.'), + project: zod.string().describe('Identifier of the project this release belongs to.'), hash_id: zod .string() .max(errorTrackingReleasesCreateBodyHashIdMax) @@ -909,23 +899,11 @@ export const ErrorTrackingReleasesCreateBody = /* @__PURE__ */ zod.object({ ), }) -export const errorTrackingReleasesUpdateBodyVersionMax = 255 - -export const errorTrackingReleasesUpdateBodyProjectMax = 255 - export const errorTrackingReleasesUpdateBodyHashIdMax = 128 export const ErrorTrackingReleasesUpdateBody = /* @__PURE__ */ zod.object({ - version: zod - .string() - .max(errorTrackingReleasesUpdateBodyVersionMax) - .nullish() - .describe('Human-readable release version. Omit to preserve the current value.'), - project: zod - .string() - .max(errorTrackingReleasesUpdateBodyProjectMax) - .nullish() - .describe('Project identifier. Omit to preserve the current value.'), + version: zod.string().nullish().describe('Human-readable release version. Omit to preserve the current value.'), + project: zod.string().nullish().describe('Project identifier. Omit to preserve the current value.'), hash_id: zod .string() .max(errorTrackingReleasesUpdateBodyHashIdMax) @@ -937,23 +915,11 @@ export const ErrorTrackingReleasesUpdateBody = /* @__PURE__ */ zod.object({ .describe('Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.'), }) -export const errorTrackingReleasesPartialUpdateBodyVersionMax = 255 - -export const errorTrackingReleasesPartialUpdateBodyProjectMax = 255 - export const errorTrackingReleasesPartialUpdateBodyHashIdMax = 128 export const ErrorTrackingReleasesPartialUpdateBody = /* @__PURE__ */ zod.object({ - version: zod - .string() - .max(errorTrackingReleasesPartialUpdateBodyVersionMax) - .nullish() - .describe('Human-readable release version. Omit to preserve the current value.'), - project: zod - .string() - .max(errorTrackingReleasesPartialUpdateBodyProjectMax) - .nullish() - .describe('Project identifier. Omit to preserve the current value.'), + version: zod.string().nullish().describe('Human-readable release version. Omit to preserve the current value.'), + project: zod.string().nullish().describe('Project identifier. Omit to preserve the current value.'), hash_id: zod .string() .max(errorTrackingReleasesPartialUpdateBodyHashIdMax) diff --git a/services/mcp/src/api/generated.ts b/services/mcp/src/api/generated.ts index e9fc8e4fe61b..c948d729f082 100644 --- a/services/mcp/src/api/generated.ts +++ b/services/mcp/src/api/generated.ts @@ -26580,15 +26580,9 @@ export namespace Schemas { export type ErrorTrackingReleaseCreateRequestMetadata = { [key: string]: unknown } | null; export interface ErrorTrackingReleaseCreateRequest { - /** - * Human-readable release version, e.g. a semver string or build number. - * @maxLength 255 - */ + /** Human-readable release version, e.g. a semver string or build number. */ version: string; - /** - * Identifier of the project this release belongs to. - * @maxLength 255 - */ + /** Identifier of the project this release belongs to. */ project: string; /** * Optional client-supplied release hash (e.g. a git commit SHA). Generated server-side when omitted. @@ -26612,13 +26606,11 @@ export namespace Schemas { export interface ErrorTrackingReleaseUpdateRequest { /** * Human-readable release version. Omit to preserve the current value. - * @maxLength 255 * @nullable */ version?: string | null; /** * Project identifier. Omit to preserve the current value. - * @maxLength 255 * @nullable */ project?: string | null; @@ -51670,13 +51662,11 @@ export namespace Schemas { export interface PatchedErrorTrackingReleaseUpdateRequest { /** * Human-readable release version. Omit to preserve the current value. - * @maxLength 255 * @nullable */ version?: string | null; /** * Project identifier. Omit to preserve the current value. - * @maxLength 255 * @nullable */ project?: string | null; From 604af1f82abdcbfaf292f090f5e38eb398ea07a6 Mon Sep 17 00:00:00 2001 From: ablaszkiewicz Date: Tue, 4 Aug 2026 13:11:41 +0200 Subject: [PATCH 9/9] refactor(error-tracking): enforce release field limits in cymbal only Drop the API-side caps on release metadata, version, and project: the public write surface stays permissive, and cymbal defends itself at fetch instead. Oversized metadata (over 8 KiB serialized) is dropped and version/project are truncated to 255 chars before a record is cached or embedded into events, so one oversized row can't be amplified across the event stream and the release cache's entry-count budget stays a real memory bound. Co-Authored-By: Claude Fable 5 --- .../backend/presentation/views/releases.py | 26 ++-------- .../tests/api/test_error_tracking_api.py | 30 ----------- .../frontend/generated/api.schemas.ts | 12 ++--- .../frontend/generated/api.zod.ts | 8 ++- rust/cymbal/src/core/types/frames/releases.rs | 52 +++++++++++++------ services/mcp/src/api/generated.ts | 12 ++--- 6 files changed, 54 insertions(+), 86 deletions(-) diff --git a/products/error_tracking/backend/presentation/views/releases.py b/products/error_tracking/backend/presentation/views/releases.py index a90efba642d6..7c25db922dfa 100644 --- a/products/error_tracking/backend/presentation/views/releases.py +++ b/products/error_tracking/backend/presentation/views/releases.py @@ -1,5 +1,3 @@ -import json - from drf_spectacular.utils import OpenApiResponse, extend_schema, extend_schema_field from rest_framework import serializers, status, viewsets from rest_framework.exceptions import NotFound, ValidationError @@ -16,10 +14,6 @@ from products.error_tracking.backend.presentation.pagination import paginate_via_facade MAX_HASH_ID_LENGTH = 128 -# Kept in sync with MAX_RELEASE_METADATA_BYTES in rust/cymbal/src/core/types/frames/releases.rs: -# cymbal embeds metadata into every matching exception event, so an unbounded value would be -# amplified across the whole event stream. -MAX_METADATA_BYTES = 8 * 1024 RELEASE_HASH_IN_USE_ERROR_CODE = "release_hash_in_use" @@ -43,9 +37,7 @@ class ErrorTrackingReleaseCreateRequestSerializer(serializers.Serializer): help_text="Optional client-supplied release hash (e.g. a git commit SHA). Generated server-side when omitted.", ) metadata = ReleaseMetadataField( - required=False, - allow_null=True, - help_text="Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON.", + required=False, allow_null=True, help_text="Optional free-form metadata object stored alongside the release." ) @@ -63,9 +55,7 @@ class ErrorTrackingReleaseUpdateRequestSerializer(serializers.Serializer): help_text="Release hash (e.g. a git commit SHA). Omit to preserve the current value.", ) metadata = ReleaseMetadataField( - required=False, - allow_null=True, - help_text="Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.", + required=False, allow_null=True, help_text="Free-form metadata object. Omit to preserve the current value." ) @@ -82,13 +72,6 @@ def _validated_hash_id(self, hash_id) -> str | None: raise ValidationError("Hash id length cannot exceed 128 bytes") return hash_id - def _validated_metadata(self, metadata: object) -> object: - if metadata is None: - return None - if len(json.dumps(metadata, separators=(",", ":")).encode("utf-8")) > MAX_METADATA_BYTES: - raise ValidationError("Metadata is too large. Keep it under 8 KB.") - return metadata - def list(self, request, *args, **kwargs) -> Response: return paginate_via_facade( self, @@ -114,14 +97,13 @@ def create(self, request, *args, **kwargs) -> Response: if not project: raise ValidationError("Project is required") hash_id = self._validated_hash_id(request.data.get("hash_id")) - metadata = self._validated_metadata(request.data.get("metadata")) try: release = error_tracking_api.create_release( self.team.id, version=str(version), project=str(project), hash_id=hash_id, - metadata=metadata, + metadata=request.data.get("metadata"), ) except error_tracking_api.ReleaseHashInUseError as err: raise ValidationError(f"Hash id {err} already in use", code=RELEASE_HASH_IN_USE_ERROR_CODE) from err @@ -133,7 +115,7 @@ def _apply_update(self, pk: str, data) -> Response: release = error_tracking_api.update_release( self.team.id, pk, - metadata=self._validated_metadata(data.get("metadata")), + metadata=data.get("metadata"), hash_id=hash_id, version=data.get("version"), project=data.get("project"), diff --git a/products/error_tracking/backend/tests/api/test_error_tracking_api.py b/products/error_tracking/backend/tests/api/test_error_tracking_api.py index 73f834754d49..2175f25b9f69 100644 --- a/products/error_tracking/backend/tests/api/test_error_tracking_api.py +++ b/products/error_tracking/backend/tests/api/test_error_tracking_api.py @@ -1277,36 +1277,6 @@ def test_releases_list_paginates_in_sql(self) -> None: ] assert limited_selects, "expected a LIMIT 2 SELECT on the release table" - @parameterized.expand(["create", "update"]) - def test_release_rejects_oversized_metadata(self, method: str) -> None: - # Cymbal embeds release metadata into every matching exception event, so an uncapped - # value would be amplified across the whole event stream. - oversized = {"blob": "x" * (8 * 1024)} - if method == "create": - response = self.client.post( - f"/api/environments/{self.team.id}/error_tracking/releases", - data={"version": "1.0.0", "project": "proj", "metadata": oversized}, - format="json", - ) - else: - release = ErrorTrackingRelease.objects.create(team=self.team, hash_id="h", version="1.0.0", project="proj") - response = self.client.patch( - f"/api/environments/{self.team.id}/error_tracking/releases/{release.id}", - data={"metadata": oversized}, - format="json", - ) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "Metadata is too large" in response.json()["detail"] - - def test_release_accepts_small_metadata(self) -> None: - response = self.client.post( - f"/api/environments/{self.team.id}/error_tracking/releases", - data={"version": "1.0.0", "project": "proj", "metadata": {"git": {"commit_id": "abc123"}}}, - format="json", - ) - assert response.status_code == status.HTTP_201_CREATED - assert response.json()["metadata"] == {"git": {"commit_id": "abc123"}} - class TestIssueStateSync(ClickhouseTestMixin, APIBaseTest): def _create_issue(self, fingerprints=None, **kwargs) -> ErrorTrackingIssue: diff --git a/products/error_tracking/frontend/generated/api.schemas.ts b/products/error_tracking/frontend/generated/api.schemas.ts index 3e8f24ad34b0..255ff2210723 100644 --- a/products/error_tracking/frontend/generated/api.schemas.ts +++ b/products/error_tracking/frontend/generated/api.schemas.ts @@ -1389,7 +1389,7 @@ export interface PaginatedErrorTrackingReleaseListApi { } /** - * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. + * Optional free-form metadata object stored alongside the release. * @nullable */ export type ErrorTrackingReleaseCreateRequestApiMetadata = { [key: string]: unknown } | null @@ -1406,14 +1406,14 @@ export interface ErrorTrackingReleaseCreateRequestApi { */ hash_id?: string | null /** - * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. + * Optional free-form metadata object stored alongside the release. * @nullable */ metadata?: ErrorTrackingReleaseCreateRequestApiMetadata } /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ export type ErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string]: unknown } | null @@ -1436,14 +1436,14 @@ export interface ErrorTrackingReleaseUpdateRequestApi { */ hash_id?: string | null /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ metadata?: ErrorTrackingReleaseUpdateRequestApiMetadata } /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ export type PatchedErrorTrackingReleaseUpdateRequestApiMetadata = { [key: string]: unknown } | null @@ -1466,7 +1466,7 @@ export interface PatchedErrorTrackingReleaseUpdateRequestApi { */ hash_id?: string | null /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ metadata?: PatchedErrorTrackingReleaseUpdateRequestApiMetadata diff --git a/products/error_tracking/frontend/generated/api.zod.ts b/products/error_tracking/frontend/generated/api.zod.ts index 0f14fc278f95..2ade6536c597 100644 --- a/products/error_tracking/frontend/generated/api.zod.ts +++ b/products/error_tracking/frontend/generated/api.zod.ts @@ -894,9 +894,7 @@ export const ErrorTrackingReleasesCreateBody = /* @__PURE__ */ zod.object({ metadata: zod .record(zod.string(), zod.unknown()) .nullish() - .describe( - 'Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON.' - ), + .describe('Optional free-form metadata object stored alongside the release.'), }) export const errorTrackingReleasesUpdateBodyHashIdMax = 128 @@ -912,7 +910,7 @@ export const ErrorTrackingReleasesUpdateBody = /* @__PURE__ */ zod.object({ metadata: zod .record(zod.string(), zod.unknown()) .nullish() - .describe('Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.'), + .describe('Free-form metadata object. Omit to preserve the current value.'), }) export const errorTrackingReleasesPartialUpdateBodyHashIdMax = 128 @@ -928,7 +926,7 @@ export const ErrorTrackingReleasesPartialUpdateBody = /* @__PURE__ */ zod.object metadata: zod .record(zod.string(), zod.unknown()) .nullish() - .describe('Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON.'), + .describe('Free-form metadata object. Omit to preserve the current value.'), }) export const ErrorTrackingSettingsUpdateSettingsPartialUpdateBody = /* @__PURE__ */ zod.object({ diff --git a/rust/cymbal/src/core/types/frames/releases.rs b/rust/cymbal/src/core/types/frames/releases.rs index 9311c3b2f4a6..ae99af412d0e 100644 --- a/rust/cymbal/src/core/types/frames/releases.rs +++ b/rust/cymbal/src/core/types/frames/releases.rs @@ -5,12 +5,15 @@ use sha2::{Digest, Sha512}; use sqlx::Executor; use uuid::Uuid; -/// Kept in sync with MAX_METADATA_BYTES in the release API -/// (products/error_tracking/backend/presentation/views/releases.py). The API rejects larger -/// metadata on write; rows predating the cap (or written outside it) are clamped at fetch so a -/// single oversized release can't be amplified into every matching event, and so cache entries -/// stay small enough for an entry-count budget. +/// The release API does not bound what a row can hold (`version`/`project`/`metadata` are +/// unbounded TextField/JSONField columns), but every one of these fields is embedded into every +/// matching exception event, so a single oversized row would be amplified across the whole event +/// stream after capture's per-event size limit has already been enforced. Clamping at fetch keeps +/// events bounded and cache entries small enough for an entry-count budget. 8 KiB of metadata is +/// ~25x what the CLI writes (a git object), and 255 chars fits any semver, commit SHA, or bundle +/// identifier many times over. pub const MAX_RELEASE_METADATA_BYTES: usize = 8 * 1024; +pub const MAX_RELEASE_TEXT_CHARS: usize = 255; #[derive(Debug, Clone, Eq, PartialEq)] pub struct ReleaseRecord { @@ -54,7 +57,7 @@ impl ReleaseRecord { .fetch_optional(e) .await?; - Ok(row.map(Self::with_clamped_metadata)) + Ok(row.map(Self::clamped)) } pub async fn for_hash<'c, E>( @@ -78,7 +81,7 @@ impl ReleaseRecord { .fetch_optional(e) .await?; - Ok(row.map(Self::with_clamped_metadata)) + Ok(row.map(Self::clamped)) } pub fn to_info(&self) -> ReleaseInfo { @@ -91,19 +94,28 @@ impl ReleaseRecord { } } - /// Drops `metadata` when its serialized form exceeds the cap the API enforces on new writes. - /// The `id` survives, so consumers can still fetch the full release. - fn with_clamped_metadata(mut self) -> Self { + /// Bounds every field this record can carry into an event: `metadata` over the cap the API + /// enforces on new writes is dropped, `version`/`project` are truncated. The `id` survives, + /// so consumers can still fetch the full release. + fn clamped(mut self) -> Self { let oversized = self.metadata.as_ref().is_some_and(|metadata| { serde_json::to_string(metadata).map_or(true, |s| s.len() > MAX_RELEASE_METADATA_BYTES) }); if oversized { self.metadata = None; } + truncate_chars(&mut self.version, MAX_RELEASE_TEXT_CHARS); + truncate_chars(&mut self.project, MAX_RELEASE_TEXT_CHARS); self } } +fn truncate_chars(value: &mut String, max_chars: usize) { + if let Some((byte_index, _)) = value.char_indices().nth(max_chars) { + value.truncate(byte_index); + } +} + /// Reconstruct the release `hash_id` the CLI wrote for a mobile build, from the app metadata the /// SDK sends on every event. Mobile events carry no injected `$release_id`, so this is how their /// release is resolved. It must stay byte-for-byte identical to the CLI, which keys releases on @@ -199,14 +211,20 @@ mod tests { } #[test] - fn oversized_metadata_is_clamped_but_small_metadata_survives() { - // Rows predating the API's write-time cap can hold multi-megabyte metadata; without the - // clamp every matching event would embed it, re-opening the amplification the cap closed. - let big = record(Some(json!({"git": {"commit_id": "x".repeat(100_000)}}))); - assert_eq!(big.with_clamped_metadata().metadata, None); + fn oversized_fields_are_clamped_but_sane_fields_survive() { + // The API does not bound these fields, and each is embedded into every matching event; + // without the clamp one oversized release row would inflate the whole event stream. + let mut big = record(Some(json!({"git": {"commit_id": "x".repeat(100_000)}}))); + big.version = "v".repeat(10_000); + big.project = "é".repeat(10_000); + let clamped = big.clamped(); + assert_eq!(clamped.metadata, None); + assert_eq!(clamped.version.chars().count(), MAX_RELEASE_TEXT_CHARS); + assert_eq!(clamped.project.chars().count(), MAX_RELEASE_TEXT_CHARS); let small_value = json!({"git": {"commit_id": "abc123"}}); - let small = record(Some(small_value.clone())); - assert_eq!(small.with_clamped_metadata().metadata, Some(small_value)); + let small = record(Some(small_value.clone())).clamped(); + assert_eq!(small.metadata, Some(small_value)); + assert_eq!(small.version, "1.0"); } } diff --git a/services/mcp/src/api/generated.ts b/services/mcp/src/api/generated.ts index c948d729f082..a5b1458c4fdc 100644 --- a/services/mcp/src/api/generated.ts +++ b/services/mcp/src/api/generated.ts @@ -26574,7 +26574,7 @@ export namespace Schemas { } /** - * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. + * Optional free-form metadata object stored alongside the release. * @nullable */ export type ErrorTrackingReleaseCreateRequestMetadata = { [key: string]: unknown } | null; @@ -26591,14 +26591,14 @@ export namespace Schemas { */ hash_id?: string | null; /** - * Optional free-form metadata object stored alongside the release. Limited to 8 KB of serialized JSON. + * Optional free-form metadata object stored alongside the release. * @nullable */ metadata?: ErrorTrackingReleaseCreateRequestMetadata; } /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ export type ErrorTrackingReleaseUpdateRequestMetadata = { [key: string]: unknown } | null; @@ -26621,7 +26621,7 @@ export namespace Schemas { */ hash_id?: string | null; /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ metadata?: ErrorTrackingReleaseUpdateRequestMetadata; @@ -51654,7 +51654,7 @@ export namespace Schemas { } /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ export type PatchedErrorTrackingReleaseUpdateRequestMetadata = { [key: string]: unknown } | null; @@ -51677,7 +51677,7 @@ export namespace Schemas { */ hash_id?: string | null; /** - * Free-form metadata object. Omit to preserve the current value. Limited to 8 KB of serialized JSON. + * Free-form metadata object. Omit to preserve the current value. * @nullable */ metadata?: PatchedErrorTrackingReleaseUpdateRequestMetadata;