Skip to content

ref(transaction): Send transaction attachments to the objectstore#5710

Open
Dav1dde wants to merge 2 commits intomasterfrom
dav1d/transaction-attachments-objectstore
Open

ref(transaction): Send transaction attachments to the objectstore#5710
Dav1dde wants to merge 2 commits intomasterfrom
dav1d/transaction-attachments-objectstore

Conversation

@Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Mar 9, 2026

Copied from the error processor, routes envelopes with attachments to the objectstore service.

@Dav1dde Dav1dde requested a review from a team as a code owner March 9, 2026 15:06
@Dav1dde Dav1dde self-assigned this Mar 9, 2026
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Duplicated objectstore routing logic across two processors
    • Extracted the duplicated forward_store logic into a default implementation on the Forward trait, eliminating the 22-line duplication between ErrorOutput and TransactionOutput.

Create PR

Or push these changes by commenting:

@cursor push 2f77e62240
Preview (2f77e62240)
diff --git a/relay-server/src/processing/errors/mod.rs b/relay-server/src/processing/errors/mod.rs
--- a/relay-server/src/processing/errors/mod.rs
+++ b/relay-server/src/processing/errors/mod.rs
@@ -336,31 +336,4 @@
             })
             .map_err(|err| err.map(|_| ()))
     }
-
-    #[cfg(feature = "processing")]
-    fn forward_store(
-        self,
-        s: processing::StoreHandle<'_>,
-        ctx: processing::ForwardContext<'_>,
-    ) -> Result<(), Rejected<()>> {
-        let envelope = self.serialize_envelope(ctx)?;
-        let envelope = ManagedEnvelope::from(envelope).into_processed();
-
-        let has_attachments = envelope
-            .envelope()
-            .items()
-            .any(|item| item.ty() == &ItemType::Attachment);
-        let use_objectstore = || {
-            let options = &ctx.global_config.options;
-            crate::utils::sample(options.objectstore_attachments_sample_rate).is_keep()
-        };
-
-        if has_attachments && use_objectstore() {
-            s.send_to_objectstore(crate::services::store::StoreEnvelope { envelope });
-        } else {
-            s.send_to_store(crate::services::store::StoreEnvelope { envelope });
-        }
-
-        Ok(())
-    }
 }

diff --git a/relay-server/src/processing/forward.rs b/relay-server/src/processing/forward.rs
--- a/relay-server/src/processing/forward.rs
+++ b/relay-server/src/processing/forward.rs
@@ -61,8 +61,37 @@
     ///
     /// This function must only be called when Relay is configured to be in processing mode.
     #[cfg(feature = "processing")]
-    fn forward_store(self, s: StoreHandle<'_>, ctx: ForwardContext<'_>)
-    -> Result<(), Rejected<()>>;
+    fn forward_store(
+        self,
+        s: StoreHandle<'_>,
+        ctx: ForwardContext<'_>,
+    ) -> Result<(), Rejected<()>>
+    where
+        Self: Sized,
+    {
+        use crate::envelope::ItemType;
+        use crate::managed::ManagedEnvelope;
+
+        let envelope = self.serialize_envelope(ctx)?;
+        let envelope = ManagedEnvelope::from(envelope).into_processed();
+
+        let has_attachments = envelope
+            .envelope()
+            .items()
+            .any(|item| item.ty() == &ItemType::Attachment);
+        let use_objectstore = || {
+            let options = &ctx.global_config.options;
+            crate::utils::sample(options.objectstore_attachments_sample_rate).is_keep()
+        };
+
+        if has_attachments && use_objectstore() {
+            s.send_to_objectstore(crate::services::store::StoreEnvelope { envelope });
+        } else {
+            s.send_to_store(crate::services::store::StoreEnvelope { envelope });
+        }
+
+        Ok(())
+    }
 }
 
 /// Context passed to [`Forward`].

diff --git a/relay-server/src/processing/transactions/types/output.rs b/relay-server/src/processing/transactions/types/output.rs
--- a/relay-server/src/processing/transactions/types/output.rs
+++ b/relay-server/src/processing/transactions/types/output.rs
@@ -65,33 +65,4 @@
             }),
         }
     }
-
-    #[cfg(feature = "processing")]
-    fn forward_store(
-        self,
-        s: StoreHandle<'_>,
-        ctx: ForwardContext<'_>,
-    ) -> Result<(), Rejected<()>> {
-        use crate::envelope::ItemType;
-
-        let envelope = self.serialize_envelope(ctx)?;
-        let envelope = ManagedEnvelope::from(envelope).into_processed();
-
-        let has_attachments = envelope
-            .envelope()
-            .items()
-            .any(|item| item.ty() == &ItemType::Attachment);
-        let use_objectstore = || {
-            let options = &ctx.global_config.options;
-            crate::utils::sample(options.objectstore_attachments_sample_rate).is_keep()
-        };
-
-        if has_attachments && use_objectstore() {
-            s.send_to_objectstore(crate::services::store::StoreEnvelope { envelope });
-        } else {
-            s.send_to_store(crate::services::store::StoreEnvelope { envelope });
-        }
-
-        Ok(())
-    }
 }
This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

s.send_to_objectstore(crate::services::store::StoreEnvelope { envelope });
} else {
s.send_to_store(crate::services::store::StoreEnvelope { envelope });
}
Copy link

Choose a reason for hiding this comment

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

Duplicated objectstore routing logic across two processors

Low Severity

The entire forward_store body (serialize → check attachments → sample objectstore → route) is now duplicated verbatim between ErrorOutput and TransactionOutput. If the routing logic ever needs a fix or new condition, both copies need identical updates — a likely source of future divergence. This could be extracted into a shared helper (e.g., a free function or a default method on the Forward trait that delegates to serialize_envelope).

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this sucks, we will address this separately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant