fix(media-proxy): serve transforms whole, never as ranges (#3446)

fluxer_media_proxy/src/server/external/mod.rs

@@ -95,7 +95,6 @@ fn external_cached_transform(
     url: &str,
     method: &Method,
     params: &HashMap<String, String>,
-    headers: &HeaderMap,
 ) -> Option<Response> {
     let hint = app.media.external_hints().get(url)?;
     let filename = url_filename(&hint.url);
@@ -109,7 +108,6 @@ fn external_cached_transform(
             filename: &filename,
             route: TransformRoute::External,
             params,
-            headers,
         },
     )
 }
@@ -295,7 +293,7 @@ pub(in crate::server) async fn serve_external(
     };
     if prepared.wants_transform
         && let Some(cached) =
-            external_cached_transform(app, &prepared.url, &prepared.method, params, headers)
+            external_cached_transform(app, &prepared.url, &prepared.method, params)
     {
         return cached;
     }

fluxer_media_proxy/src/server/response/mod.rs

@@ -69,6 +69,41 @@ pub(in crate::server) fn media_response(response: MediaResponse<'_>) -> Response
     )
 }
 
+pub(in crate::server) struct GeneratedMediaResponse<'a> {
+    pub(in crate::server) method: Method,
+    pub(in crate::server) data: BudgetedBytes,
+    pub(in crate::server) content_type: &'a str,
+    pub(in crate::server) disposition: Option<HeaderValue>,
+}
+
+pub(in crate::server) fn generated_media_response(
+    response: GeneratedMediaResponse<'_>,
+) -> Response {
+    let GeneratedMediaResponse {
+        method,
+        data,
+        content_type,
+        disposition,
+    } = response;
+    let total_len = data.len();
+    let body = if method == Method::HEAD {
+        Body::empty()
+    } else {
+        Body::from(retained_response_bytes(data))
+    };
+    let mut response = media_body_response(
+        body,
+        StatusCode::OK,
+        total_len,
+        content_type,
+        None,
+        Some(total_len as u64),
+        disposition,
+    );
+    response.headers_mut().remove(header::ACCEPT_RANGES);
+    response
+}
+
 pub(in crate::server) fn media_body_response(
     body: Body,
     status: StatusCode,

fluxer_media_proxy/src/server/routes/internal.rs

@@ -14,11 +14,11 @@ use crate::{
             load_media_input, read_sniff_prefix, resolve_metadata,
         },
         response::{
-            MediaResponse,
+            GeneratedMediaResponse,
             error::{
                 canonical_reason_str, json_response, storage_error_response, text, text_with_source,
             },
-            media_response,
+            generated_media_response,
         },
         state::AppState,
         transform::execution::{
@@ -231,11 +231,10 @@ pub(in crate::server) async fn thumbnail_handler(
             }
         }
     };
-    media_response(MediaResponse {
+    generated_media_response(GeneratedMediaResponse {
         method: Method::GET,
         data: BudgetedBytes::from(Bytes::from(media.bytes)),
         content_type: media.content_type,
-        range_header: None,
         disposition: None,
     })
 }

fluxer_media_proxy/src/server/stored/mod.rs

@@ -27,9 +27,9 @@ use crate::{
             filename_from_storage_key,
         },
         response::{
-            MediaResponse, content_disposition_header,
+            GeneratedMediaResponse, MediaResponse, content_disposition_header,
             error::{storage_error_response, text_with_source},
-            media_response,
+            generated_media_response, media_response,
         },
         state::AppState,
         transform::{
@@ -146,11 +146,10 @@ pub(in crate::server) async fn serve_asset_image(
         resize_mode: Some(resize_mode),
     });
     if let Some(hit) = cached_transform_hit(runtime, &cache_key) {
-        return media_response(MediaResponse {
+        return generated_media_response(GeneratedMediaResponse {
             method,
             data: hit.data.as_bytes().clone().into(),
             content_type: hit.format.mime(),
-            range_header,
             disposition: Some(content_disposition_header(
                 hit.format.mime(),
                 requested_download,
@@ -202,11 +201,10 @@ pub(in crate::server) async fn serve_asset_image(
             });
         }
     };
-    media_response(MediaResponse {
+    generated_media_response(GeneratedMediaResponse {
         method,
         data: transformed.into(),
         content_type: out_ext.mime(),
-        range_header,
         disposition: Some(content_disposition_header(
             out_ext.mime(),
             requested_download,
@@ -390,7 +388,6 @@ async fn serve_spooled_attachment_video(
                 filename,
                 route: TransformRoute::Attachment,
                 params,
-                headers,
             },
         )
     };

fluxer_media_proxy/src/server/stored/passthrough.rs

@@ -18,8 +18,8 @@ use crate::{
             content_type_is_trustworthy, image_extension_from_filename, is_svg_content_type,
         },
         response::{
-            MediaResponse, content_disposition_header, error::storage_error_response,
-            error::text_with_source, media_response,
+            GeneratedMediaResponse, content_disposition_header, error::storage_error_response,
+            error::text_with_source, generated_media_response,
         },
         state::AppState,
         transform::{
@@ -86,7 +86,7 @@ pub(in crate::server) async fn serve_stored_passthrough_stream(
     if app.cfg.mode == DeploymentMode::Mp
         && image_extension_from_filename(key) == Some(AssetExtension::Svg)
     {
-        return serve_stored_passthrough_svg(app, method, bucket, key, headers, &disposition).await;
+        return serve_stored_passthrough_svg(app, method, bucket, key, &disposition).await;
     }
     let range_header = headers.get(header::RANGE).and_then(|v| v.to_str().ok());
     let forwarded_range = match range::classify_request_range(range_header) {
@@ -105,7 +105,7 @@ pub(in crate::server) async fn serve_stored_passthrough_stream(
     }
     let content_type = passthrough_content_type(&object.content_type, key);
     if app.cfg.mode == DeploymentMode::Mp && is_svg_content_type(&content_type) {
-        return serve_stored_passthrough_svg(app, method, bucket, key, headers, &disposition).await;
+        return serve_stored_passthrough_svg(app, method, bucket, key, &disposition).await;
     }
     let total_len = match passthrough_total_len(app, bucket, key, object.total_length).await {
         Ok(value) => value,
@@ -137,8 +137,7 @@ async fn serve_stored_passthrough_head(
         && (is_svg_content_type(&content_type)
             || image_extension_from_filename(key) == Some(AssetExtension::Svg))
     {
-        return serve_stored_passthrough_svg(app, Method::HEAD, bucket, key, headers, disposition)
-            .await;
+        return serve_stored_passthrough_svg(app, Method::HEAD, bucket, key, disposition).await;
     }
     let total_len = match usize::try_from(head.content_length) {
         Ok(value) => value,
@@ -163,7 +162,6 @@ async fn serve_stored_passthrough_svg(
     method: Method,
     bucket: &str,
     key: &str,
-    headers: &HeaderMap,
     disposition: &PassthroughDisposition<'_>,
 ) -> Response {
     let object = match app.store.read_object(bucket, key).await {
@@ -171,15 +169,7 @@ async fn serve_stored_passthrough_svg(
         Err(err) => return storage_error_response(key, err),
     };
     let cache_identity = format!("{bucket}/{key}");
-    serve_stored_svg_rasterized(
-        app,
-        method,
-        object.data,
-        &cache_identity,
-        headers,
-        disposition,
-    )
-    .await
+    serve_stored_svg_rasterized(app, method, object.data, &cache_identity, disposition).await
 }
 
 /// Resolves the object's full length, preferring what the store already reported on the streamed
@@ -259,7 +249,6 @@ async fn serve_stored_svg_rasterized(
     method: Method,
     data: Bytes,
     cache_identity: &str,
-    headers: &HeaderMap,
     disposition: &PassthroughDisposition<'_>,
 ) -> Response {
     let runtime = app.media.transforms();
@@ -287,11 +276,10 @@ async fn serve_stored_svg_rasterized(
         resize_mode: Some(options.resize_mode),
     });
     if let Some(hit) = cached_transform_hit(runtime, &cache_key) {
-        return media_response(MediaResponse {
+        return generated_media_response(GeneratedMediaResponse {
             method,
             data: hit.data.as_bytes().clone().into(),
             content_type: hit.format.mime(),
-            range_header: headers.get(header::RANGE).and_then(|v| v.to_str().ok()),
             disposition: passthrough_disposition_header(disposition, hit.format.mime()),
         });
     }
@@ -322,11 +310,10 @@ async fn serve_stored_svg_rasterized(
             );
         }
     };
-    media_response(MediaResponse {
+    generated_media_response(GeneratedMediaResponse {
         method,
         data: transformed.into(),
         content_type: format.mime(),
-        range_header: headers.get(header::RANGE).and_then(|v| v.to_str().ok()),
         disposition: passthrough_disposition_header(disposition, format.mime()),
     })
 }

fluxer_media_proxy/src/server/stored/tests.rs

@@ -230,6 +230,47 @@ async fn an_asset_image_transform_is_served_and_then_reused_from_its_content_ide
     );
 }
 
+#[tokio::test]
+async fn a_ranged_request_for_an_asset_transform_is_answered_with_the_whole_body() {
+    let tmp = tempfile::tempdir().unwrap();
+    let storage_root = tmp.path().canonicalize().unwrap();
+    let app = test_app_state(asset_test_config(&storage_root));
+    let asset = parse_standard_asset_path(ASSET_TEST_PATH).unwrap();
+    app.store
+        .write_object(
+            &app.cfg.storage.bucket_cdn,
+            &asset.storage_key,
+            &crate::test_fixtures::synthetic_png(512, 512),
+            "image/png",
+        )
+        .await
+        .unwrap();
+    let params = HashMap::new();
+    let mut range = HeaderMap::new();
+    range.insert(header::RANGE, "bytes=0-3".parse().unwrap());
+    let mut bodies = Vec::new();
+    for headers in [&range, &range, &HeaderMap::new()] {
+        let response = serve_asset_image(
+            &app,
+            Method::GET,
+            parse_standard_asset_path(ASSET_TEST_PATH).unwrap(),
+            &params,
+            headers,
+        )
+        .await;
+        assert_eq!(StatusCode::OK, response.status());
+        assert!(!response.headers().contains_key(header::ACCEPT_RANGES));
+        assert!(!response.headers().contains_key(header::CONTENT_RANGE));
+        bodies.push(response_body(response).await);
+    }
+    assert!(bodies[2].len() > 4);
+    assert_eq!(bodies[2], bodies[0]);
+    assert_eq!(bodies[2], bodies[1]);
+    let rendered = app.metrics.render();
+    assert!(rendered.contains("fluxer_media_proxy_transform_cache_misses_total 1\n"));
+    assert!(rendered.contains("fluxer_media_proxy_transform_cache_hits_total 2\n"));
+}
+
 #[test]
 fn a_transform_failure_on_a_source_the_browser_cannot_show_stays_an_error() {
     let svg_failure = |error| {
@@ -381,6 +422,70 @@ async fn a_stored_passthrough_serves_objects_larger_than_the_buffer_cap() {
     assert_eq!(&b"head"[..], &response_body(get).await[..]);
 }
 
+#[tokio::test]
+async fn a_ranged_request_for_a_rasterised_stored_svg_is_answered_with_the_whole_body() {
+    let tmp = tempfile::tempdir().unwrap();
+    let storage_root = tmp.path().canonicalize().unwrap();
+    let app = test_app_state(asset_test_config(&storage_root));
+    let bucket = app.cfg.storage.bucket_cdn.clone();
+    let key = "attachments/1/2/pic.svg";
+    app.store
+        .write_object(
+            &bucket,
+            key,
+            br#"<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8"><rect width="8" height="8" fill="red"/></svg>"#,
+            "image/svg+xml",
+        )
+        .await
+        .unwrap();
+    let mut range = HeaderMap::new();
+    range.insert(header::RANGE, "bytes=0-3".parse().unwrap());
+    let mut past_the_end = HeaderMap::new();
+    past_the_end.insert(header::RANGE, "bytes=1048576-2097151".parse().unwrap());
+    let mut bodies = Vec::new();
+    for (method, headers) in [
+        (Method::GET, &range),
+        (Method::GET, &past_the_end),
+        (Method::HEAD, &range),
+        (Method::GET, &HeaderMap::new()),
+    ] {
+        let response = passthrough::serve_stored_raw(&app, method, &bucket, key, headers).await;
+        assert_eq!(StatusCode::OK, response.status());
+        assert_eq!(
+            Some("image/webp"),
+            response
+                .headers()
+                .get(header::CONTENT_TYPE)
+                .and_then(|value| value.to_str().ok())
+        );
+        assert!(!response.headers().contains_key(header::ACCEPT_RANGES));
+        assert!(!response.headers().contains_key(header::CONTENT_RANGE));
+        let declared_len = response
+            .headers()
+            .get(header::CONTENT_LENGTH)
+            .and_then(|value| value.to_str().ok())
+            .and_then(|value| value.parse::<usize>().ok());
+        bodies.push((declared_len, response_body(response).await));
+    }
+    let [
+        (fresh_len, fresh_body),
+        (cached_len, cached_body),
+        (head_len, head_body),
+        (whole_len, whole_body),
+    ] = bodies.try_into().expect("four responses");
+    assert_eq!("image/webp", mime::sniff(&whole_body).mime);
+    assert_eq!(Some(whole_body.len()), whole_len);
+    assert_eq!(whole_len, fresh_len);
+    assert_eq!(whole_len, cached_len);
+    assert_eq!(whole_len, head_len);
+    assert_eq!(whole_body, fresh_body);
+    assert_eq!(whole_body, cached_body);
+    assert!(head_body.is_empty());
+    let rendered = app.metrics.render();
+    assert!(rendered.contains("fluxer_media_proxy_transform_cache_misses_total 1\n"));
+    assert!(rendered.contains("fluxer_media_proxy_transform_cache_hits_total 3\n"));
+}
+
 fn poster_params() -> HashMap<String, String> {
     HashMap::from([
         ("format".to_owned(), "webp".to_owned()),

fluxer_media_proxy/src/server/transform/mod.rs

@@ -31,9 +31,9 @@ use crate::{
         },
         native_task_executor::{NativeTaskExecutor, NativeTaskExecutorSettings},
         response::{
-            MediaResponse, content_disposition_header,
+            GeneratedMediaResponse, MediaResponse, content_disposition_header,
             error::{text_with_reason, text_with_source},
-            media_response,
+            generated_media_response, media_response,
         },
         transform::{
             cache::{
@@ -350,7 +350,6 @@ pub(in crate::server) struct CachedVideoTransformRequest<'a> {
     pub(in crate::server) filename: &'a str,
     pub(in crate::server) route: TransformRoute,
     pub(in crate::server) params: &'a HashMap<String, String>,
-    pub(in crate::server) headers: &'a HeaderMap,
 }
 
 pub(in crate::server) fn cached_video_transform_response(
@@ -371,10 +370,6 @@ pub(in crate::server) fn cached_video_transform_response(
         data: hit.data.as_bytes().clone(),
         content_type: hit.format.mime(),
         filename: request.filename,
-        range_header: request
-            .headers
-            .get(header::RANGE)
-            .and_then(|v| v.to_str().ok()),
         requested_download: transform.requested_download,
     }))
 }
@@ -432,7 +427,6 @@ async fn serve_video_transform(
             data: hit.data.as_bytes().clone(),
             content_type: hit.format.mime(),
             filename,
-            range_header,
             requested_download: transform.requested_download,
         });
     }
@@ -486,7 +480,6 @@ async fn serve_video_transform(
         data: transformed,
         content_type: format.mime(),
         filename,
-        range_header,
         requested_download: transform.requested_download,
     })
 }
@@ -571,7 +564,6 @@ async fn serve_image_transform(
             data: hit.data.as_bytes().clone(),
             content_type: response_content_type,
             filename,
-            range_header,
             requested_download: transform.requested_download,
         });
     }
@@ -636,7 +628,6 @@ async fn serve_image_transform(
         data: transformed,
         content_type: response_content_type,
         filename,
-        range_header,
         requested_download: transform.requested_download,
     })
 }
@@ -725,7 +716,6 @@ pub(in crate::server) struct CachedTransformProbe<'a> {
     pub(in crate::server) filename: &'a str,
     pub(in crate::server) route: TransformRoute,
     pub(in crate::server) params: &'a HashMap<String, String>,
-    pub(in crate::server) headers: &'a HeaderMap,
 }
 
 // Answers a repeat transform from the cache without the caller fetching the source
@@ -742,7 +732,6 @@ pub(in crate::server) fn cached_transform_response(
         filename,
         route,
         params,
-        headers,
     } = probe;
     let transform = validate_transform_parameters(params, &runtime.limits(), route).ok()?;
     let source_is_svg = is_svg_content_type(content_type)
@@ -787,7 +776,6 @@ pub(in crate::server) fn cached_transform_response(
         data: hit.data.as_bytes().clone(),
         content_type: &response_content_type,
         filename,
-        range_header: headers.get(header::RANGE).and_then(|v| v.to_str().ok()),
         requested_download: transform.requested_download,
     }))
 }
@@ -797,7 +785,6 @@ struct TransformedBytes<'a> {
     data: Bytes,
     content_type: &'a str,
     filename: &'a str,
-    range_header: Option<&'a str>,
     requested_download: bool,
 }
 
@@ -807,11 +794,10 @@ fn transformed_response(request: TransformedBytes<'_>) -> Response {
         request.requested_download,
         Some(request.filename),
     ));
-    media_response(MediaResponse {
+    generated_media_response(GeneratedMediaResponse {
         method: request.method,
         data: request.data.into(),
         content_type: request.content_type,
-        range_header: request.range_header,
         disposition,
     })
 }

fluxer_media_proxy/src/server/transform/tests.rs

@@ -335,6 +335,81 @@ async fn a_full_size_jxl_above_the_static_output_cap_is_downscaled() {
     );
 }
 
+async fn jxl_attachment(
+    runtime: &TransformRuntime,
+    method: Method,
+    params: &HashMap<String, String>,
+    headers: &HeaderMap,
+) -> Response {
+    serve_bytes_or_transform(
+        runtime,
+        ServeBytesRequest {
+            method,
+            data: Bytes::from_static(JXL_64_LOSSY_ALPHA),
+            content_type: "image/jxl".to_owned(),
+            cache_identity: "art.jxl",
+            filename: "art.jxl",
+            route: TransformRoute::Attachment,
+            params,
+            headers,
+        },
+    )
+    .await
+}
+
+#[tokio::test]
+async fn a_ranged_request_for_a_transform_is_answered_with_the_whole_body() {
+    let runtime = test_runtime();
+    let transform = HashMap::from([("format".to_owned(), "webp".to_owned())]);
+    let mut ranged = HeaderMap::new();
+    ranged.insert(header::RANGE, "bytes=0-3".parse().unwrap());
+    let mut bodies = Vec::new();
+    for (method, headers) in [
+        (Method::GET, &ranged),
+        (Method::HEAD, &ranged),
+        (Method::GET, &HeaderMap::new()),
+    ] {
+        let response = jxl_attachment(&runtime, method, &transform, headers).await;
+        assert_eq!(StatusCode::OK, response.status());
+        assert!(!response.headers().contains_key(header::ACCEPT_RANGES));
+        assert!(!response.headers().contains_key(header::CONTENT_RANGE));
+        let declared_len = response
+            .headers()
+            .get(header::CONTENT_LENGTH)
+            .and_then(|value| value.to_str().ok())
+            .and_then(|value| value.parse::<usize>().ok());
+        let body = to_bytes(response.into_body(), usize::MAX)
+            .await
+            .expect("body");
+        bodies.push((declared_len, body));
+    }
+    let [
+        (ranged_len, ranged_body),
+        (head_len, head_body),
+        (whole_len, whole_body),
+    ] = bodies.try_into().expect("three responses");
+    assert!(whole_body.len() > 4);
+    assert_eq!(Some(whole_body.len()), whole_len);
+    assert_eq!(whole_len, ranged_len);
+    assert_eq!(whole_len, head_len);
+    assert_eq!(whole_body, ranged_body);
+    assert!(head_body.is_empty());
+
+    let original = jxl_attachment(&runtime, Method::GET, &HashMap::new(), &ranged).await;
+    assert_eq!(StatusCode::PARTIAL_CONTENT, original.status());
+    assert_eq!(
+        Some("bytes"),
+        original
+            .headers()
+            .get(header::ACCEPT_RANGES)
+            .and_then(|value| value.to_str().ok())
+    );
+    let original_body = to_bytes(original.into_body(), usize::MAX)
+        .await
+        .expect("body");
+    assert_eq!(&JXL_64_LOSSY_ALPHA[..4], &original_body[..]);
+}
+
 #[test]
 fn the_budget_leaves_room_for_the_cache_and_headroom() {
     assert_eq!(