From 242c5ce01cd3388f7d23a87b68615b2042a04bfc Mon Sep 17 00:00:00 2001 From: chess Date: Thu, 10 Sep 2026 17:56:37 +0000 Subject: [PATCH] Preserve whole diagnostic attachments and report incomplete uploads (#44606) ## Why Truncating diagnostic attachments can corrupt compressed archives, while skipping unreadable files can hide incomplete diagnostic delivery. ## What changed - Remove attachment truncation and the 40,000,000-byte wire cap. Enforce the 200 MiB decoded envelope limit, including framing, and report oversized or unreadable attachments as upload failures. - Use gzip only when it reduces envelope size, and set `Content-Encoding` according to the request bytes for both interactive and report uploads. - Document that successful uploads mean HTTP acceptance, not durable attachment storage, and that callers must retain source files. ## Testing Add coverage for incomplete uploads caused by missing or oversized files. Update large-file coverage to verify byte-for-byte delivery of JSONL files and a gzip archive exceeding the former wire cap, including successful archive decompression. GitOrigin-RevId: a5316e9f407f16ee5a3dfb3f15122b1db51d6a94 --- .../feedback/src/attachment_truncation.rs | 41 ---- codex-rs/feedback/src/lib.rs | 211 ++++++++++-------- codex-rs/feedback/src/report_upload.rs | 7 +- .../feedback/src/rollout_attachment_tests.rs | 25 +-- codex-rs/feedback/src/upload.rs | 91 ++++---- 5 files changed, 160 insertions(+), 215 deletions(-) delete mode 100644 codex-rs/feedback/src/attachment_truncation.rs diff --git a/codex-rs/feedback/src/attachment_truncation.rs b/codex-rs/feedback/src/attachment_truncation.rs deleted file mode 100644 index ef8fc3dd7c..0000000000 --- a/codex-rs/feedback/src/attachment_truncation.rs +++ /dev/null @@ -1,41 +0,0 @@ -use std::path::Path; - -use anyhow::Context; -use anyhow::Result; - -/// Shorten and label an attachment copy, leaving files within the target untouched. -/// JSONL keeps at least one complete record when a smaller record prefix is possible, -/// even if it exceeds the target. Callers must check the resulting upload size. -/// Other formats use a byte prefix. -pub(super) fn truncate_attachment( - filename: &mut String, - buffer: &mut Vec, - target_bytes: usize, -) -> Result<()> { - if buffer.len() <= target_bytes { - return Ok(()); - } - - let end = match Path::new(filename) - .extension() - .and_then(|extension| extension.to_str()) - { - Some(extension) if extension.eq_ignore_ascii_case("jsonl") => buffer[..target_bytes] - .iter() - .rposition(|byte| *byte == b'\n') - .or_else(|| { - buffer - .iter() - .position(|byte| *byte == b'\n') - .filter(|end| *end + 1 < buffer.len()) - }) - .map(|end| end + 1) - .context("feedback attachment cannot be shortened to a complete JSONL record")?, - _ => target_bytes, - }; - buffer.truncate(end); - if !filename.starts_with("truncated-") { - filename.insert_str(0, "truncated-"); - } - Ok(()) -} diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index 7c86bfefd0..2bf038d95f 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -30,7 +30,6 @@ use tracing_subscriber::filter::Targets; use tracing_subscriber::fmt::writer::MakeWriter; use tracing_subscriber::registry::LookupSpan; -mod attachment_truncation; pub(crate) mod feedback_diagnostics; mod guardian; mod report_upload; @@ -60,12 +59,12 @@ const UPLOAD_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 300); // Raw collection budgets used by the report API, not the interactive upload. pub const MAX_ATTACHMENT_BYTES: usize = 64 * 1024 * 1024; pub const MAX_ATTACHMENTS_BYTES: usize = 126 * 1024 * 1024; -// Check complete envelopes against Sentry's published limits, including framing: +// Bound decoded envelopes, including framing. Do not shorten attachments to fit: +// they may already be compressed, and callers need an error for incomplete delivery. +// This bounds the decoded request including framing, not guaranteed attachment storage. // https://develop.sentry.dev/sdk/foundations/envelopes/#size-limits -// https://docs.sentry.io/platforms/javascript/enriching-events/attachments/ const MAX_DECODED_UPLOAD_BYTES: usize = 200 * 1024 * 1024; const MAX_EVENT_BYTES: usize = 1024 * 1024; -const MAX_UPLOAD_BYTES: usize = 40_000_000; const FEEDBACK_TAGS_TARGET: &str = "feedback_tags"; const MAX_FEEDBACK_TAGS: usize = 64; @@ -390,11 +389,6 @@ pub struct FeedbackAttachmentPath { pub attachment_filename_override: Option, } -enum AttachmentReadMode { - Whole, - Prefix, -} - #[cfg(test)] #[path = "rollout_attachment_tests.rs"] mod rollout_attachment_tests; @@ -402,14 +396,6 @@ mod rollout_attachment_tests; impl FeedbackAttachmentPath { /// Read a whole regular file within the caller's size limit. pub fn read_attachment(&self, max_bytes: usize) -> io::Result> { - self.read_attachment_with_mode(max_bytes, AttachmentReadMode::Whole) - } - - fn read_attachment_with_mode( - &self, - max_bytes: usize, - mode: AttachmentReadMode, - ) -> io::Result> { let rollout_path = codex_rollout::rollout_id_from_path(&self.path) .map(|_| codex_rollout::plain_rollout_path(&self.path)); let buffer = if rollout_path.is_some() { @@ -421,20 +407,17 @@ impl FeedbackAttachmentPath { buffer } else { let metadata = fs::metadata(&self.path)?; - if !metadata.is_file() - || (metadata.len() > max_bytes as u64 && matches!(mode, AttachmentReadMode::Whole)) - { + if !metadata.is_file() || metadata.len() > max_bytes as u64 { return Ok(None); } let mut buffer = Vec::new(); - // Keep one extra byte so the encoder can detect and label a truncated prefix, - // including when the file grows after the metadata check. + // Detect files that grow past the limit after the metadata check. fs::File::open(&self.path)? .take(max_bytes as u64 + 1) .read_to_end(&mut buffer)?; buffer }; - if buffer.len() > max_bytes && matches!(mode, AttachmentReadMode::Whole) { + if buffer.len() > max_bytes { return Ok(None); } let filename = self @@ -578,7 +561,10 @@ impl FeedbackSnapshot { self.feedback_diagnostics.attachment_text() } - /// Upload feedback to Sentry with optional attachments. + /// Submit feedback to Sentry with whole attachments within a shared deadline. + /// Success means HTTP acceptance, not durable attachment storage. Callers must + /// retain their source files; errors can occur after part of a report is accepted. + /// https://github.com/getsentry/relay/blob/master/relay-server/src/endpoints/common.rs pub async fn upload_feedback( &self, options: FeedbackUploadOptions<'_>, @@ -618,7 +604,7 @@ impl FeedbackSnapshot { let mut envelope = Envelope::new(); envelope.add_item(EnvelopeItem::Event(event)); let (event_body, event_bytes) = - upload::gzip_envelope(&envelope).context("failed to serialize feedback event")?; + upload::encode_envelope(&envelope).context("failed to serialize feedback event")?; anyhow::ensure!( event_bytes <= MAX_EVENT_BYTES, "feedback event exceeds the size limit" @@ -631,7 +617,7 @@ impl FeedbackSnapshot { ); // Accept the report before reading diagnostics; all envelopes share one deadline. let mut rate_limited = false; - let status = upload::send_gzip_envelope( + let status = upload::send_envelope( &client_pool, &dsn, event_body, @@ -669,8 +655,8 @@ impl FeedbackSnapshot { } let mut status = None; let result: Result<()> = async { - let body = upload::gzip_attachment_envelope(&headers, attachment)?; - let response_status = upload::send_gzip_envelope( + let body = upload::encode_attachment_envelope(&headers, attachment?)?; + let response_status = upload::send_envelope( &client_pool, &dsn, body, @@ -761,12 +747,12 @@ impl FeedbackSnapshot { extra_attachments: &'a [FeedbackAttachment], extra_attachment_paths: &'a [FeedbackAttachmentPath], logs_override: Option>, - ) -> impl Iterator + 'a { + ) -> impl Iterator> + 'a { use sentry::protocol::Attachment; // Priority: logs, generated attachments (doctor report), connectivity diagnostics, - // then files in caller order. Read and compress each file independently; - // raw sizes across separate requests do not determine whether their gzip bodies fit. + // then files in caller order. Measure each file’s envelope independently; + // the size limit applies per request, not to the sum of all files. let logs = include_logs.then(|| self.log_attachment(logs_override)); let diagnostics = self .feedback_diagnostics_attachment_text(include_logs) @@ -787,30 +773,19 @@ impl FeedbackSnapshot { }), ) .chain(diagnostics) - .chain(extra_attachment_paths.iter().filter_map(|attachment_path| { - match attachment_path - .read_attachment_with_mode(MAX_DECODED_UPLOAD_BYTES, AttachmentReadMode::Prefix) - { - Ok(Some(attachment)) => Some(attachment), - Ok(None) => { - tracing::warn!("feedback attachment is not a regular file; skipping"); - None - } - Err(err) => { - tracing::warn!( - path = %attachment_path.path.display(), - error = %err, - "failed to read log attachment; skipping" - ); - None - } - } + .map(Ok) + .chain(extra_attachment_paths.iter().map(|attachment_path| { + attachment_path + .read_attachment(MAX_DECODED_UPLOAD_BYTES)? + .context("feedback attachment is not a regular file or exceeds the size limit") })) - .map(|attachment| Attachment { - buffer: attachment.buffer, - filename: attachment.filename, - content_type: attachment.content_type, - ty: None, + .map(|attachment| { + attachment.map(|attachment| Attachment { + buffer: attachment.buffer, + filename: attachment.filename, + content_type: attachment.content_type, + ty: None, + }) }) } } @@ -910,7 +885,9 @@ mod tests { use super::*; use crate::FeedbackDiagnostic; use codex_http_client::OutboundProxyPolicy; + use flate2::Compression; use flate2::read::GzDecoder; + use flate2::write::GzEncoder; use http::StatusCode; use pretty_assertions::assert_eq; use tracing_subscriber::layer::SubscriberExt; @@ -1148,7 +1125,10 @@ mod tests { .expect_err("legacy uploads report incomplete diagnostics"); assert!(started.elapsed() >= Duration::from_secs(2)); let requests = server.received_requests().await.unwrap(); - assert_eq!(requests[1].body, requests[2].body, "retry exact gzip bytes"); + assert_eq!( + requests[1].body, requests[2].body, + "retry exact request bytes" + ); // Exhausting one diagnostic must not delay later.txt or replay earlier files. assert!( requests[3..6] @@ -1163,22 +1143,22 @@ mod tests { } #[tokio::test] - async fn feedback_upload_delivers_whole_files_or_marked_prefixes() { + async fn feedback_upload_delivers_large_files_intact() { let suffix = ThreadId::new(); let first_path = std::env::temp_dir().join(format!("feedback-first-{suffix}.jsonl")); let second_path = std::env::temp_dir().join(format!("feedback-second-{suffix}.jsonl")); - let binary_path = std::env::temp_dir().join(format!("feedback-binary-{suffix}.bin")); + let binary_path = std::env::temp_dir().join(format!("feedback-archive-{suffix}.jsonl.gz")); let pending_path = second_path.with_extension("pending"); let block = "{\"type\":\"event_msg\",\"payload\":{\"type\":\"agent_message\",\"message\":\"diagnostic fixture 🍵\"}}\n" .repeat(1024); - let block_counts = [65, 205].map(|mib| mib * 1024 * 1024 / block.len() + 1); + let block_counts = [65, 80].map(|mib| mib * 1024 * 1024 / block.len() + 1); for (path, blocks) in [&first_path, &pending_path].into_iter().zip(block_counts) { let mut file = fs::File::create(path).unwrap(); for _ in 0..blocks { file.write_all(block.as_bytes()).unwrap(); } } - // Repeats beyond gzip's 32 KiB window keep this fixture above the wire limit. + // Repeats beyond gzip's window keep the archive above the former 38.1 MiB cap. let record_block = (0..4096) .flat_map(|_| sentry::types::Uuid::new_v4().into_bytes()) .map(|byte| { @@ -1186,9 +1166,7 @@ mod tests { [usize::from(byte % 64)] }) .collect::>(); - let mut file = fs::File::create(&binary_path).unwrap(); - // The first record fits compressed, but exceeds the encoder's initial - // half-file target. It must survive when this same file is sent as JSONL. + let mut file = GzEncoder::new(fs::File::create(&binary_path).unwrap(), Compression::fast()); for mib in [32, 30] { file.write_all(b"{\"message\":\"").unwrap(); for _ in 0..mib * 1024 * 1024 / record_block.len() { @@ -1196,14 +1174,14 @@ mod tests { } file.write_all(b"\"}\n").unwrap(); } - drop(file); + file.finish().unwrap(); + assert!(fs::metadata(&binary_path).unwrap().len() > 40_000_000); let server = MockServer::start().await; let attempt = AtomicUsize::default(); let ready_path = second_path.clone(); Mock::given(method("POST")) .and(path("/api/42/envelope/")) - .and(header("Content-Encoding", "gzip")) .respond_with(move |_: &wiremock::Request| { if attempt.fetch_add(/*val*/ 1, Ordering::SeqCst) == 1 { // The next file becomes readable only after the first file arrives. @@ -1211,7 +1189,7 @@ mod tests { } ResponseTemplate::new(StatusCode::OK) }) - .expect(/*r*/ 5) + .expect(/*r*/ 4) .mount(&server) .await; @@ -1237,12 +1215,6 @@ mod tests { path: binary_path.clone(), attachment_filename_override: None, }, - FeedbackAttachmentPath { - path: binary_path.clone(), - attachment_filename_override: Some(format!( - "feedback-records-{suffix}.jsonl" - )), - }, ], session_source: Some(SessionSource::Cli), logs_override: None, @@ -1254,31 +1226,24 @@ mod tests { .await .unwrap(); let requests = server.received_requests().await.unwrap(); - assert_eq!(requests.len(), 5); + assert_eq!(requests.len(), 4); let mut event = String::new(); GzDecoder::new(requests[0].body.as_slice()) .read_to_string(&mut event) .unwrap(); for (request, (filename, source_path)) in requests[1..].iter().zip([ (format!("feedback-first-{suffix}.jsonl"), &first_path), - ( - format!("truncated-feedback-second-{suffix}.jsonl"), - &second_path, - ), - ( - format!("truncated-feedback-binary-{suffix}.bin"), - &binary_path, - ), - ( - format!("truncated-feedback-records-{suffix}.jsonl"), - &binary_path, - ), + (format!("feedback-second-{suffix}.jsonl"), &second_path), + (format!("feedback-archive-{suffix}.jsonl.gz"), &binary_path), ]) { - assert!(request.body.len() <= MAX_UPLOAD_BYTES); let mut decoded = Vec::new(); - GzDecoder::new(request.body.as_slice()) - .read_to_end(&mut decoded) - .unwrap(); + if request.headers.contains_key("Content-Encoding") { + GzDecoder::new(request.body.as_slice()) + .read_to_end(&mut decoded) + .unwrap(); + } else { + decoded.extend_from_slice(&request.body); + } assert!(decoded.len() <= MAX_DECODED_UPLOAD_BYTES); let mut parts = decoded.splitn(3, |byte| *byte == b'\n'); assert_eq!( @@ -1290,14 +1255,13 @@ mod tests { let payload = parts.next().unwrap().strip_suffix(b"\n").unwrap(); let mut source = fs::File::open(source_path).unwrap(); let original_bytes = source.metadata().unwrap().len() as usize; - if filename.starts_with("truncated-") { - assert!(!payload.is_empty() && payload.len() < original_bytes); - } else { - assert_eq!(payload.len(), original_bytes); - } + assert_eq!(payload.len(), original_bytes); if filename.ends_with(".jsonl") { let text = std::str::from_utf8(payload).unwrap(); assert!(text.ends_with('\n')); + } else { + let mut archive = GzDecoder::new(payload); + io::copy(&mut archive, &mut io::sink()).expect("received archive must be intact"); } let mut expected = [0; 64 * 1024]; for chunk in payload.chunks(expected.len()) { @@ -1310,6 +1274,58 @@ mod tests { fs::remove_file(binary_path).unwrap(); } + #[tokio::test] + async fn feedback_upload_reports_unreadable_or_oversized_attachments() { + let oversized_path = + std::env::temp_dir().join(format!("feedback-oversized-{}", ThreadId::new())); + fs::File::create(&oversized_path) + .unwrap() + .set_len(MAX_DECODED_UPLOAD_BYTES as u64 + 1) + .unwrap(); + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(StatusCode::OK)) + .expect(/*r*/ 2) + .mount(&server) + .await; + + for path in [ + oversized_path.clone(), + oversized_path.with_extension("missing"), + ] { + let result = CodexFeedback::new() + .snapshot(/*session_id*/ None) + .upload_feedback_with_dsn( + FeedbackUploadOptions { + classification: "bug", + reason: None, + tags: None, + include_logs: false, + extra_attachments: &[], + extra_attachment_paths: &[FeedbackAttachmentPath { + path, + attachment_filename_override: None, + }], + session_source: Some(SessionSource::Cli), + logs_override: None, + }, + &HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), + &format!("http://public@{}/42", server.address()), + Instant::now() + UPLOAD_TIMEOUT, + ) + .await; + assert_eq!( + result.unwrap_err().to_string(), + "feedback report was accepted, but some attachments failed to upload" + ); + } + assert_eq!( + fs::metadata(&oversized_path).unwrap().len(), + MAX_DECODED_UPLOAD_BYTES as u64 + 1 + ); + fs::remove_file(oversized_path).unwrap(); + } + #[tokio::test] async fn feedback_upload_stops_diagnostics_for_sentry_rate_limits() { for (accepted_before_limit, rate_limit) in [ @@ -1472,7 +1488,8 @@ mod tests { &[extra_attachment_path], Some(vec![1]), ) - .collect::>(); + .collect::>>() + .unwrap(); assert_eq!( attachments_with_diagnostics @@ -1508,7 +1525,8 @@ mod tests { .snapshot(/*session_id*/ None) .with_feedback_diagnostics(FeedbackDiagnostics::default()) .feedback_attachments(/*include_logs*/ true, &[], &[], Some(vec![1])) - .collect::>(); + .collect::>>() + .unwrap(); assert_eq!( attachments_without_diagnostics @@ -1550,7 +1568,8 @@ mod tests { ], /*logs_override*/ None, ) - .collect::>(); + .collect::>>() + .unwrap(); fs::remove_file(gzip_path).expect("gzip attachment should be removed"); fs::remove_file(unknown_path).expect("unknown attachment should be removed"); diff --git a/codex-rs/feedback/src/report_upload.rs b/codex-rs/feedback/src/report_upload.rs index 1410832c7e..56f9ee54a2 100644 --- a/codex-rs/feedback/src/report_upload.rs +++ b/codex-rs/feedback/src/report_upload.rs @@ -20,7 +20,6 @@ use crate::FeedbackAttachment; use crate::FeedbackSnapshot; use crate::MAX_DECODED_UPLOAD_BYTES; use crate::MAX_EVENT_BYTES; -use crate::MAX_UPLOAD_BYTES; use crate::SENTRY_DSN; use crate::upload; @@ -63,7 +62,7 @@ impl FeedbackTransport { /// Make one attempt. Callers own retry decisions and must retain the same bytes. pub async fn send(&self, envelope: Vec) -> FeedbackDelivery { - let response = upload::gzip_envelope_request( + let response = upload::envelope_request( &self.client_pool, &self.dsn, envelope.into(), @@ -146,9 +145,9 @@ pub fn prepare_report_attachment( fn prepare_envelope(envelope: Envelope, max_decoded_bytes: usize) -> Result> { let (bytes, decoded_bytes) = - upload::gzip_envelope(&envelope).context("failed to serialize feedback envelope")?; + upload::encode_envelope(&envelope).context("failed to serialize feedback envelope")?; anyhow::ensure!( - bytes.len() <= MAX_UPLOAD_BYTES && decoded_bytes <= max_decoded_bytes, + decoded_bytes <= max_decoded_bytes, "feedback envelope exceeds the size limit" ); Ok(bytes) diff --git a/codex-rs/feedback/src/rollout_attachment_tests.rs b/codex-rs/feedback/src/rollout_attachment_tests.rs index b74a3a9dee..537ffbad1e 100644 --- a/codex-rs/feedback/src/rollout_attachment_tests.rs +++ b/codex-rs/feedback/src/rollout_attachment_tests.rs @@ -59,7 +59,8 @@ fn unloaded_compressed_rollout_is_included_as_jsonl_attachment() { &paths, /*logs_override*/ None, ) - .collect::>(); + .collect::>>() + .unwrap(); assert_eq!( attachments .iter() @@ -174,7 +175,7 @@ fn queued_attachments_follow_both_representation_transitions() { } #[test] -fn compressed_attachment_bounds_decoded_bytes_and_preserves_jsonl_truncation() { +fn compressed_attachment_bounds_decoded_bytes() { let fixture = Fixture::new(); let path = fixture.attachment(); assert!(path.read_attachment(/*max_bytes*/ 35).unwrap().is_none()); @@ -182,26 +183,6 @@ fn compressed_attachment_bounds_decoded_bytes_and_preserves_jsonl_truncation() { path.read_attachment(JSONL.len()).unwrap().unwrap().buffer, JSONL ); - // The valid first frame provides the needed prefix. Decoding the entire file would fail. - fs::write( - &fixture.compressed, - [COMPRESSED, b"invalid trailing frame"].concat(), - ) - .unwrap(); - let mut attachment = path - .read_attachment_with_mode(/*max_bytes*/ 35, AttachmentReadMode::Prefix) - .unwrap() - .unwrap(); - assert_eq!(attachment.buffer, JSONL[..36]); - crate::attachment_truncation::truncate_attachment( - &mut attachment.filename, - &mut attachment.buffer, - /*target_bytes*/ 35, - ) - .unwrap(); - assert_eq!(attachment.buffer, b"{\"message\":\"old diagnostic\"}\n"); - assert!(attachment.filename.starts_with("truncated-rollout-")); - assert!(attachment.filename.ends_with(".jsonl")); assert!(!fixture.plain.exists()); } diff --git a/codex-rs/feedback/src/upload.rs b/codex-rs/feedback/src/upload.rs index dd62543dca..0c9722b181 100644 --- a/codex-rs/feedback/src/upload.rs +++ b/codex-rs/feedback/src/upload.rs @@ -17,11 +17,10 @@ use sentry::ClientOptions; use sentry::protocol::Attachment; use sentry::protocol::Envelope; use sentry::protocol::EnvelopeHeaders; +use sentry::protocol::EnvelopeItem; use sentry::types::Dsn; use crate::MAX_DECODED_UPLOAD_BYTES; -use crate::MAX_UPLOAD_BYTES; -use crate::attachment_truncation::truncate_attachment; pub(super) const DEFAULT_RATE_LIMIT: Duration = Duration::from_secs(/*secs*/ 60); @@ -30,7 +29,7 @@ pub(super) enum EnvelopeKind { Attachment, } -pub(super) fn gzip_envelope_request( +pub(super) fn envelope_request( client_pool: &RouteAwareClientPool, dsn: &Dsn, body: Bytes, @@ -38,16 +37,21 @@ pub(super) fn gzip_envelope_request( ) -> RouteAwareRequestBuilder { let sentry_options = ClientOptions::default(); let sentry_auth = dsn.to_auth(Some(sentry_options.user_agent.as_ref())); - client_pool + let request = client_pool .post(dsn.envelope_api_url().as_str()) - .header("X-Sentry-Auth", sentry_auth.to_string()) - .header("Content-Encoding", "gzip") - .body(body) - .timeout(timeout) + .header("X-Sentry-Auth", sentry_auth.to_string()); + // Persisted parts retain their encoding. Gzip's magic bytes cannot begin + // an uncompressed envelope, which starts with a JSON header. + let request = if body.starts_with(&[0x1f, 0x8b]) { + request.header("Content-Encoding", "gzip") + } else { + request + }; + request.body(body).timeout(timeout) } /// Send an already-serialized envelope within the report's shared deadline. -pub(super) async fn send_gzip_envelope( +pub(super) async fn send_envelope( client_pool: &RouteAwareClientPool, dsn: &Dsn, body: Vec, @@ -56,12 +60,12 @@ pub(super) async fn send_gzip_envelope( rate_limited: &mut bool, ) -> Result { let body = Bytes::from(body); - // Retain the exact gzip bytes for at most two diagnostic retries. Never replay the core. + // Retain the exact request bytes for at most two diagnostic retries. Never replay the core. let mut retry_delays = [250, 500].map(Duration::from_millis).into_iter(); let response = loop { let timeout = deadline.saturating_duration_since(Instant::now()); anyhow::ensure!(!timeout.is_zero(), "feedback upload deadline exceeded"); - let response = gzip_envelope_request(client_pool, dsn, body.clone(), timeout) + let response = envelope_request(client_pool, dsn, body.clone(), timeout) .send() .await; if response.as_ref().is_ok_and(|response| { @@ -158,56 +162,39 @@ pub(super) fn parse_retry_after(value: &str) -> Option { .ok() } -pub(super) fn gzip_envelope(envelope: &Envelope) -> io::Result<(Vec, usize)> { +pub(super) fn encode_envelope(envelope: &Envelope) -> io::Result<(Vec, usize)> { let mut writer = CountingWriter { inner: GzEncoder::new(Vec::new(), Compression::fast()), bytes: 0, }; envelope.to_writer(&mut writer)?; - Ok((writer.inner.finish()?, writer.bytes)) + let decoded_bytes = writer.bytes; + let mut body = writer.inner.finish()?; + // Already-compressed attachments can grow beyond Sentry's 200 MiB wire limit. + // Reuse the buffer for the raw envelope when gzip does not make it smaller. + if body.len() >= decoded_bytes { + body.clear(); + envelope.to_writer(&mut body)?; + } + Ok((body, decoded_bytes)) } -pub(super) fn gzip_attachment_envelope( +pub(super) fn encode_attachment_envelope( headers: &EnvelopeHeaders, - mut attachment: Attachment, + attachment: Attachment, ) -> Result> { - let envelope = Envelope::new().with_headers(headers.clone()); - // Apply the file reader's bound to in-memory attachments too. The extra byte - // lets the format-aware truncation below detect and label a shortened copy. - attachment.buffer.truncate(MAX_DECODED_UPLOAD_BYTES + 1); - let mut target_bytes = MAX_DECODED_UPLOAD_BYTES; - loop { - truncate_attachment( - &mut attachment.filename, - &mut attachment.buffer, - target_bytes, - )?; - let mut writer = CountingWriter { - inner: GzEncoder::new(Vec::new(), Compression::fast()), - bytes: 0, - }; - // Use the SDK's framing while retaining the buffer for in-place truncation. - // https://github.com/getsentry/sentry-rust/blob/0.46.1/sentry-types/src/protocol/envelope.rs#L453-L487 - envelope.to_writer(&mut writer)?; - attachment.to_writer(&mut writer)?; - writeln!(writer)?; - let decoded_bytes = writer.bytes; - let body = writer.inner.finish()?; - if body.len() <= MAX_UPLOAD_BYTES && decoded_bytes <= MAX_DECODED_UPLOAD_BYTES { - return Ok(body); - } - anyhow::ensure!( - !attachment.buffer.is_empty(), - "feedback attachment headers exceed the size limit" - ); - target_bytes = attachment - .buffer - .len() - .saturating_sub(decoded_bytes.saturating_sub(MAX_DECODED_UPLOAD_BYTES)); - if body.len() > MAX_UPLOAD_BYTES { - target_bytes /= 2; - } - } + anyhow::ensure!( + attachment.buffer.len() <= MAX_DECODED_UPLOAD_BYTES, + "feedback attachment exceeds the size limit" + ); + let mut envelope = Envelope::new().with_headers(headers.clone()); + envelope.add_item(EnvelopeItem::Attachment(attachment)); + let (body, decoded_bytes) = encode_envelope(&envelope)?; + anyhow::ensure!( + decoded_bytes <= MAX_DECODED_UPLOAD_BYTES, + "feedback attachment envelope exceeds the size limit" + ); + Ok(body) } struct CountingWriter {