fix(thumbnail): add timeout protection for large image processing
Resolves conflict between main and PR #229 by applying timeout support on top of main's drop(data) memory optimization. Changes: - Add thumbnail_generation_ms to TimeoutConfig (default 30s) - Add generation_timeout field to ThumbnailService - Wrap spawn_blocking in tokio::time::timeout to prevent hanging - Update DI to pass timeout from config - Update tests to provide timeout parameter https://claude.ai/code/session_015FD62aAoMYx1XBZbzPvUu8
This commit is contained in:
@@ -34,6 +34,8 @@ pub struct TimeoutConfig {
|
|||||||
pub lock_acquisition_ms: u64,
|
pub lock_acquisition_ms: u64,
|
||||||
/// Timeout for network operations (ms)
|
/// Timeout for network operations (ms)
|
||||||
pub network_operation_ms: u64,
|
pub network_operation_ms: u64,
|
||||||
|
/// Timeout for thumbnail generation (ms)
|
||||||
|
pub thumbnail_generation_ms: u64,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Default for TimeoutConfig {
|
impl Default for TimeoutConfig {
|
||||||
@@ -43,6 +45,7 @@ impl Default for TimeoutConfig {
|
|||||||
dir_operation_ms: 30000, // 30 seconds
|
dir_operation_ms: 30000, // 30 seconds
|
||||||
lock_acquisition_ms: 5000, // 5 seconds
|
lock_acquisition_ms: 5000, // 5 seconds
|
||||||
network_operation_ms: 15000, // 15 seconds
|
network_operation_ms: 15000, // 15 seconds
|
||||||
|
thumbnail_generation_ms: 30000, // 30 seconds
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -82,6 +85,11 @@ impl TimeoutConfig {
|
|||||||
pub fn network_timeout(&self) -> Duration {
|
pub fn network_timeout(&self) -> Duration {
|
||||||
Duration::from_millis(self.network_operation_ms)
|
Duration::from_millis(self.network_operation_ms)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Gets a Duration for thumbnail generation operations
|
||||||
|
pub fn thumbnail_timeout(&self) -> Duration {
|
||||||
|
Duration::from_millis(self.thumbnail_generation_ms)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Configuration for large resource handling
|
/// Configuration for large resource handling
|
||||||
|
|||||||
+2
-1
@@ -119,12 +119,13 @@ impl AppServiceFactory {
|
|||||||
}));
|
}));
|
||||||
tracing::info!("FileContentCache initialized: max 10MB/file, 512MB total, 10k entries");
|
tracing::info!("FileContentCache initialized: max 10MB/file, 512MB total, 10k entries");
|
||||||
|
|
||||||
// Thumbnail service for thumbnail generation
|
// Thumbnail service for thumbnail generation with timeout protection
|
||||||
let thumbnail_service = Arc::new(
|
let thumbnail_service = Arc::new(
|
||||||
crate::infrastructure::services::thumbnail_service::ThumbnailService::new(
|
crate::infrastructure::services::thumbnail_service::ThumbnailService::new(
|
||||||
&self.storage_path,
|
&self.storage_path,
|
||||||
5000, // max 5000 thumbnails in cache
|
5000, // max 5000 thumbnails in cache
|
||||||
100 * 1024 * 1024, // max 100MB cache
|
100 * 1024 * 1024, // max 100MB cache
|
||||||
|
Some(self.config.timeouts.thumbnail_timeout()),
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
// Initialize thumbnail directories
|
// Initialize thumbnail directories
|
||||||
|
|||||||
@@ -12,11 +12,14 @@ use image::imageops::FilterType;
|
|||||||
* - JPEG output (lossy q=80) for compact thumbnails
|
* - JPEG output (lossy q=80) for compact thumbnails
|
||||||
* - Lock-free moka cache with weight-based eviction
|
* - Lock-free moka cache with weight-based eviction
|
||||||
* - Lazy generation on first request if not pre-generated
|
* - Lazy generation on first request if not pre-generated
|
||||||
|
* - Timeout protection for large image processing
|
||||||
*/
|
*/
|
||||||
use std::path::{Path, PathBuf};
|
use std::path::{Path, PathBuf};
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
use std::time::Duration;
|
||||||
use tokio::fs;
|
use tokio::fs;
|
||||||
use tokio::sync::Semaphore;
|
use tokio::sync::Semaphore;
|
||||||
|
use tokio::time::timeout;
|
||||||
|
|
||||||
use crate::application::ports::thumbnail_ports::{
|
use crate::application::ports::thumbnail_ports::{
|
||||||
ThumbnailPort, ThumbnailSize as PortThumbnailSize, ThumbnailStatsDto,
|
ThumbnailPort, ThumbnailSize as PortThumbnailSize, ThumbnailStatsDto,
|
||||||
@@ -90,6 +93,9 @@ pub struct ThumbnailService {
|
|||||||
/// Without this, 50 simultaneous uploads would decode 50 bitmaps
|
/// Without this, 50 simultaneous uploads would decode 50 bitmaps
|
||||||
/// (~96 MB each for 6000×4000) = 4.8 GB peak.
|
/// (~96 MB each for 6000×4000) = 4.8 GB peak.
|
||||||
decode_semaphore: Arc<Semaphore>,
|
decode_semaphore: Arc<Semaphore>,
|
||||||
|
/// Timeout for thumbnail generation operations to prevent hanging on large images.
|
||||||
|
/// Defaults to 30 seconds.
|
||||||
|
generation_timeout: Duration,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ThumbnailService {
|
impl ThumbnailService {
|
||||||
@@ -99,7 +105,13 @@ impl ThumbnailService {
|
|||||||
/// * `storage_root` - Root path of file storage
|
/// * `storage_root` - Root path of file storage
|
||||||
/// * `max_cache_entries` - (ignored — moka uses weight-based eviction)
|
/// * `max_cache_entries` - (ignored — moka uses weight-based eviction)
|
||||||
/// * `max_cache_bytes` - Maximum total bytes to cache
|
/// * `max_cache_bytes` - Maximum total bytes to cache
|
||||||
pub fn new(storage_root: &Path, max_cache_entries: usize, max_cache_bytes: usize) -> Self {
|
/// * `generation_timeout` - Timeout for thumbnail generation operations
|
||||||
|
pub fn new(
|
||||||
|
storage_root: &Path,
|
||||||
|
max_cache_entries: usize,
|
||||||
|
max_cache_bytes: usize,
|
||||||
|
generation_timeout: Option<Duration>,
|
||||||
|
) -> Self {
|
||||||
let thumbnails_root = storage_root.join(".thumbnails");
|
let thumbnails_root = storage_root.join(".thumbnails");
|
||||||
|
|
||||||
// Ignore max_cache_entries — weight-based eviction is more accurate
|
// Ignore max_cache_entries — weight-based eviction is more accurate
|
||||||
@@ -123,6 +135,7 @@ impl ThumbnailService {
|
|||||||
cache,
|
cache,
|
||||||
max_cache_bytes: max_cache_bytes as u64,
|
max_cache_bytes: max_cache_bytes as u64,
|
||||||
decode_semaphore: Arc::new(Semaphore::new(DEFAULT_MAX_CONCURRENT_DECODES)),
|
decode_semaphore: Arc::new(Semaphore::new(DEFAULT_MAX_CONCURRENT_DECODES)),
|
||||||
|
generation_timeout: generation_timeout.unwrap_or(Duration::from_secs(30)),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -351,6 +364,7 @@ impl ThumbnailService {
|
|||||||
/// capped at `MAX_DECODE_PIXELS` to reject pathologically large images.
|
/// capped at `MAX_DECODE_PIXELS` to reject pathologically large images.
|
||||||
/// After decoding, the encoded image buffer is explicitly dropped before
|
/// After decoding, the encoded image buffer is explicitly dropped before
|
||||||
/// processing to minimize peak memory usage.
|
/// processing to minimize peak memory usage.
|
||||||
|
/// A timeout prevents the operation from hanging indefinitely on large images.
|
||||||
async fn generate_thumbnail(
|
async fn generate_thumbnail(
|
||||||
&self,
|
&self,
|
||||||
original_path: &Path,
|
original_path: &Path,
|
||||||
@@ -358,6 +372,7 @@ impl ThumbnailService {
|
|||||||
) -> Result<Bytes, ThumbnailError> {
|
) -> Result<Bytes, ThumbnailError> {
|
||||||
let path = original_path.to_path_buf();
|
let path = original_path.to_path_buf();
|
||||||
let max_dim = size.max_dimension();
|
let max_dim = size.max_dimension();
|
||||||
|
let timeout_duration = self.generation_timeout;
|
||||||
|
|
||||||
// Acquire semaphore permit — bounds peak RAM from concurrent decodes
|
// Acquire semaphore permit — bounds peak RAM from concurrent decodes
|
||||||
let _permit = self
|
let _permit = self
|
||||||
@@ -366,8 +381,9 @@ impl ThumbnailService {
|
|||||||
.await
|
.await
|
||||||
.map_err(|_| ThumbnailError::TaskError("Decode semaphore closed".into()))?;
|
.map_err(|_| ThumbnailError::TaskError("Decode semaphore closed".into()))?;
|
||||||
|
|
||||||
// Run image processing in blocking thread pool
|
// Run image processing in blocking thread pool with timeout
|
||||||
let result = tokio::task::spawn_blocking(move || -> Result<Vec<u8>, ThumbnailError> {
|
let spawn_result =
|
||||||
|
tokio::task::spawn_blocking(move || -> Result<Vec<u8>, ThumbnailError> {
|
||||||
// Single read: load file once into memory, then work from the buffer
|
// Single read: load file once into memory, then work from the buffer
|
||||||
let data =
|
let data =
|
||||||
std::fs::read(&path).map_err(|e| ThumbnailError::ImageError(e.to_string()))?;
|
std::fs::read(&path).map_err(|e| ThumbnailError::ImageError(e.to_string()))?;
|
||||||
@@ -430,8 +446,17 @@ impl ThumbnailService {
|
|||||||
.map_err(|e| ThumbnailError::ImageError(e.to_string()))?;
|
.map_err(|e| ThumbnailError::ImageError(e.to_string()))?;
|
||||||
|
|
||||||
Ok(buffer)
|
Ok(buffer)
|
||||||
})
|
});
|
||||||
|
|
||||||
|
// Apply timeout to prevent hanging on large images
|
||||||
|
let result = timeout(timeout_duration, spawn_result)
|
||||||
.await
|
.await
|
||||||
|
.map_err(|_| {
|
||||||
|
ThumbnailError::TaskError(format!(
|
||||||
|
"Thumbnail generation timed out after {:?}",
|
||||||
|
timeout_duration
|
||||||
|
))
|
||||||
|
})?
|
||||||
.map_err(|e| ThumbnailError::TaskError(e.to_string()))?;
|
.map_err(|e| ThumbnailError::TaskError(e.to_string()))?;
|
||||||
|
|
||||||
result.map(Bytes::from)
|
result.map(Bytes::from)
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
use std::time::Duration;
|
||||||
|
|
||||||
use super::thumbnail_service::{ThumbnailService, ThumbnailSize};
|
use super::thumbnail_service::{ThumbnailService, ThumbnailSize};
|
||||||
|
|
||||||
@@ -29,7 +30,12 @@ async fn generate_thumbnail_from_blob_path() {
|
|||||||
let blob_path = blob_dir.join("ab1234567890.blob");
|
let blob_path = blob_dir.join("ab1234567890.blob");
|
||||||
std::fs::write(&blob_path, tiny_png()).expect("write test blob");
|
std::fs::write(&blob_path, tiny_png()).expect("write test blob");
|
||||||
|
|
||||||
let svc = Arc::new(ThumbnailService::new(storage_root, 100, 10 * 1024 * 1024));
|
let svc = Arc::new(ThumbnailService::new(
|
||||||
|
storage_root,
|
||||||
|
100,
|
||||||
|
10 * 1024 * 1024,
|
||||||
|
Some(Duration::from_secs(30)),
|
||||||
|
));
|
||||||
svc.initialize().await.expect("init thumbnail dirs");
|
svc.initialize().await.expect("init thumbnail dirs");
|
||||||
|
|
||||||
// The key assertion: the service can read from a blob path (not a logical path)
|
// The key assertion: the service can read from a blob path (not a logical path)
|
||||||
@@ -51,7 +57,12 @@ async fn generate_thumbnail_from_blob_path() {
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn generate_thumbnail_nonexistent_path_returns_error() {
|
async fn generate_thumbnail_nonexistent_path_returns_error() {
|
||||||
let tmp = tempfile::tempdir().expect("create temp dir");
|
let tmp = tempfile::tempdir().expect("create temp dir");
|
||||||
let svc = Arc::new(ThumbnailService::new(tmp.path(), 100, 10 * 1024 * 1024));
|
let svc = Arc::new(ThumbnailService::new(
|
||||||
|
tmp.path(),
|
||||||
|
100,
|
||||||
|
10 * 1024 * 1024,
|
||||||
|
Some(Duration::from_secs(30)),
|
||||||
|
));
|
||||||
svc.initialize().await.expect("init thumbnail dirs");
|
svc.initialize().await.expect("init thumbnail dirs");
|
||||||
|
|
||||||
let bad_path = tmp.path().join("does-not-exist.png");
|
let bad_path = tmp.path().join("does-not-exist.png");
|
||||||
|
|||||||
Reference in New Issue
Block a user