From 5bd5d7ad899a37f7edf935369a7a1eff63e53aff Mon Sep 17 00:00:00 2001
From: Oliver Browne <oliver@posthog.com>
Date: Thu, 28 Nov 2024 01:37:57 +0200
Subject: [PATCH] fix(err): discard old frame results (#26495)

---
 rust/cymbal/src/config.rs          |  8 ++++++++
 rust/cymbal/src/frames/records.rs  |  8 +++++++-
 rust/cymbal/src/frames/resolver.rs | 16 ++++++++++------
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/rust/cymbal/src/config.rs b/rust/cymbal/src/config.rs
index 66d5ca26482eb..23968d02d1c4a 100644
--- a/rust/cymbal/src/config.rs
+++ b/rust/cymbal/src/config.rs
@@ -75,6 +75,14 @@ pub struct Config {
     #[envconfig(default = "600")]
     pub frame_cache_ttl_seconds: u64,
 
+    // When we resolve a frame, we put it in PG, so other instances of cymbal can
+    // use it, or so we can re-use it after a restart. This is the TTL for that,
+    // after this many minutes we'll discard saved resolution results and re-resolve
+    // TODO - 10 minutes is too short for production use, it's only twice as long as
+    // our in-memory caching. We should do at least an hour once we release
+    #[envconfig(default = "10")]
+    pub frame_result_ttl_minutes: u32,
+
     // Maximum number of lines of pre and post context to get per frame
     #[envconfig(default = "15")]
     pub context_line_count: usize,
diff --git a/rust/cymbal/src/frames/records.rs b/rust/cymbal/src/frames/records.rs
index ef17a0f991b04..05e4afa7dd4bc 100644
--- a/rust/cymbal/src/frames/records.rs
+++ b/rust/cymbal/src/frames/records.rs
@@ -1,4 +1,4 @@
-use chrono::{DateTime, Utc};
+use chrono::{DateTime, Duration, Utc};
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use sqlx::Executor;
@@ -75,6 +75,7 @@ impl ErrorTrackingStackFrame {
         e: E,
         team_id: i32,
         raw_id: &str,
+        result_ttl: Duration,
     ) -> Result<Option<Self>, UnhandledError>
     where
         E: Executor<'c, Database = sqlx::Postgres>,
@@ -105,6 +106,11 @@ impl ErrorTrackingStackFrame {
             return Ok(None);
         };
 
+        // If frame results are older than an hour or so, we discard them (and overwrite them)
+        if found.created_at < Utc::now() - result_ttl {
+            return Ok(None);
+        }
+
         // We don't serialise frame contexts on the Frame itself, but save it on the frame record,
         // and so when we load a frame record we need to patch back up the context onto the frame,
         // since we dropped it when we serialised the frame during saving.
diff --git a/rust/cymbal/src/frames/resolver.rs b/rust/cymbal/src/frames/resolver.rs
index 11564c4950a21..c1d4fc34633f4 100644
--- a/rust/cymbal/src/frames/resolver.rs
+++ b/rust/cymbal/src/frames/resolver.rs
@@ -13,6 +13,7 @@ use super::{records::ErrorTrackingStackFrame, Frame, RawFrame};
 
 pub struct Resolver {
     cache: Cache<String, ErrorTrackingStackFrame>,
+    result_ttl: chrono::Duration,
 }
 
 impl Resolver {
@@ -21,7 +22,9 @@ impl Resolver {
             .time_to_live(Duration::from_secs(config.frame_cache_ttl_seconds))
             .build();
 
-        Self { cache }
+        let result_ttl = chrono::Duration::minutes(config.frame_result_ttl_minutes as i64);
+
+        Self { cache, result_ttl }
     }
 
     pub async fn resolve(
@@ -36,7 +39,7 @@ impl Resolver {
         }
 
         if let Some(result) =
-            ErrorTrackingStackFrame::load(pool, team_id, &frame.frame_id()).await?
+            ErrorTrackingStackFrame::load(pool, team_id, &frame.frame_id(), self.result_ttl).await?
         {
             self.cache.insert(frame.frame_id(), result.clone());
             return Ok(result.contents);
@@ -237,10 +240,11 @@ mod test {
 
         // get the frame
         let frame_id = frame.frame_id();
-        let frame = ErrorTrackingStackFrame::load(&pool, 0, &frame_id)
-            .await
-            .unwrap()
-            .unwrap();
+        let frame =
+            ErrorTrackingStackFrame::load(&pool, 0, &frame_id, chrono::Duration::minutes(30))
+                .await
+                .unwrap()
+                .unwrap();
 
         assert_eq!(frame.symbol_set_id.unwrap(), set.id);