fix(upload): stream chapter pages to storage instead of buffering the whole chapter
The chapter upload handler read every `page` part fully into a Vec before
writing any, so peak memory was the whole chapter (bounded only by the
200 MiB body limit and amplified by concurrent uploads). It also accepted
an unbounded number of pages.
Stream each page part to a `staging/{upload_id}/…` key as it arrives — at
most one page's bytes are held at a time — then, once the chapter row (and
its id) exists, promote each staged blob to its final key via a new
`Storage::rename` (LocalStorage: fs rename; default impl: stream+delete for
future backends). Finalization is all-or-nothing: on any failure the DB
rolls back and both staged and already-finalized blobs are cleaned up.
Add MAX_PAGES_PER_CHAPTER (UploadConfig, default 2000, 0 = disabled),
rejecting an over-cap upload with 413 before any DB write. Also document
the crawler-side CRAWLER_MAX_IMAGES_PER_CHAPTER (added earlier) in
.env.example + docker-compose so the env-coverage test passes.
Tests: LocalStorage rename unit tests; a 413 over-cap upload test; existing
rollback + happy-path upload tests still green (the fault-injecting storage
counts put/put_stream, so mid-upload failure still rolls back).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -127,6 +127,23 @@ impl Storage for LocalStorage {
|
||||
}
|
||||
}
|
||||
|
||||
async fn rename(&self, from: &str, to: &str) -> Result<(), StorageError> {
|
||||
let from_path = self.resolve(from)?;
|
||||
let to_path = self.resolve(to)?;
|
||||
// Create the destination parent so a rename into a not-yet-existing
|
||||
// chapter directory succeeds. `from` and `to` share the storage
|
||||
// root (same filesystem), so this is a cheap atomic metadata move,
|
||||
// not a copy.
|
||||
if let Some(parent) = to_path.parent() {
|
||||
fs::create_dir_all(parent).await?;
|
||||
}
|
||||
match fs::rename(&from_path, &to_path).await {
|
||||
Ok(()) => Ok(()),
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => Err(StorageError::NotFound),
|
||||
Err(e) => Err(e.into()),
|
||||
}
|
||||
}
|
||||
|
||||
async fn exists(&self, key: &str) -> Result<bool, StorageError> {
|
||||
let path: &Path = &self.resolve(key)?;
|
||||
Ok(fs::try_exists(path).await?)
|
||||
@@ -273,6 +290,34 @@ mod tests {
|
||||
assert_eq!(entries, vec!["ok.bin"]);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rename_moves_blob_and_creates_destination_dirs() {
|
||||
let dir = tempdir().unwrap();
|
||||
let s = LocalStorage::new(dir.path());
|
||||
s.put("staging/up/0000.png", b"page-bytes").await.unwrap();
|
||||
|
||||
// Destination dir doesn't exist yet — rename must create it.
|
||||
s.rename("staging/up/0000.png", "mangas/m/chapters/c/pages/0001.png")
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(!s.exists("staging/up/0000.png").await.unwrap(), "source gone");
|
||||
assert_eq!(
|
||||
s.get("mangas/m/chapters/c/pages/0001.png").await.unwrap(),
|
||||
b"page-bytes"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rename_missing_source_is_not_found() {
|
||||
let dir = tempdir().unwrap();
|
||||
let s = LocalStorage::new(dir.path());
|
||||
assert!(matches!(
|
||||
s.rename("staging/nope.png", "dest/x.png").await,
|
||||
Err(StorageError::NotFound)
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_stream_emits_multiple_chunks_for_large_files() {
|
||||
use futures_util::StreamExt as _;
|
||||
|
||||
@@ -82,6 +82,27 @@ pub trait Storage: Send + Sync {
|
||||
async fn delete(&self, key: &str) -> Result<(), StorageError>;
|
||||
async fn exists(&self, key: &str) -> Result<bool, StorageError>;
|
||||
|
||||
/// Move a blob from `from` to `to`, overwriting any existing blob at
|
||||
/// `to`. Returns `NotFound` if `from` doesn't exist. The chapter
|
||||
/// upload path uses this to promote a staged page to its final,
|
||||
/// chapter-scoped key once the chapter row (and thus its id) exists —
|
||||
/// so pages can be streamed to storage as their multipart parts
|
||||
/// arrive, without buffering the whole chapter in memory.
|
||||
///
|
||||
/// The default implementation streams `from` to `to` and deletes the
|
||||
/// source, so backends without a native move still satisfy the
|
||||
/// contract. LocalStorage overrides it with a filesystem rename (an
|
||||
/// atomic metadata op within a mount); a future S3Storage would
|
||||
/// override with a server-side copy + delete.
|
||||
async fn rename(&self, from: &str, to: &str) -> Result<(), StorageError> {
|
||||
use futures_util::StreamExt as _;
|
||||
let StreamingFile { stream, .. } = self.get_stream(from).await?;
|
||||
let mapped: PutByteStream<'_> = Box::pin(stream.map(|r| r.map_err(StorageError::Io)));
|
||||
self.put_stream(to, mapped).await?;
|
||||
self.delete(from).await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Size in bytes of the blob at `key`, `NotFound` when it doesn't
|
||||
/// exist. Cheap metadata lookup (local: `fs::metadata`; a future
|
||||
/// `S3Storage`: HEAD object). Used by the cover-capture path and the
|
||||
|
||||
Reference in New Issue
Block a user