feat(uploads): chunked resumable image upload with JP2 ingest pipeline - #133
Open
saad-mhmd wants to merge 7 commits into
Open
feat(uploads): chunked resumable image upload with JP2 ingest pipeline#133saad-mhmd wants to merge 7 commits into
saad-mhmd wants to merge 7 commits into
Conversation
…ld (#72) - Add nullable technical-metadata fields to ItemImage (width, height, source_format, size_bytes, checksum_sha256, original_path, exif, uploaded_by, created, modified). The migrated corpus keeps NULLs — no synthetic backfill. Merge migration 0024 reconciles this branch's 0023 with main's 0023_msdescarea sibling leaf. - Fix the backoffice 400 bug: DRF auto-mapped the IIIFField to a binary ImageField, rejecting the JSON path strings the edit dialog sends. The new ImagePathField accepts ONLY media-relative path strings and rejects raw file uploads, so byte ingestion must go through the normalizing upload pipeline (JPEG-in-TIFF served unconverted is the issue #114 failure class). - Document the management item-image write operations in schema.yaml (they existed undocumented) and the new fields; tags reads as an array (tagulous list), not a string. - Register ItemImage for EditEvent auditing (new events only). - Add .jp2/.tiff to the image-picker extension whitelist — the corpus is .jp2 since #114 but the filesystem picker could not list it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#72) New apps.uploads: superuser-only chunked uploads of manuscript images (TIFF/JPEG/PNG/JP2, up to UPLOADS_MAX_BYTES) targeted at an ItemPart, normalized to lossless JP2 before SIPI ever serves them. Flow: POST /api/v1/uploads/sessions/ (validates type, size, subfolder safety, destination collisions across disk + ItemImage rows + active sessions, and free disk) -> PUT sessions/{id}/chunks/{n}/ (raw octet-stream, ~100 MB each, idempotent, resumable via missing_chunks) -> POST finalize/ (sha256 + size verified) -> Celery ingest task: inspect (Pillow), convert (vips CLI --lossless, Pillow fallback, mirrors scripts/convert_tif_to_jp2.py), smoke-test a REAL SIPI tile (info.json 200s on undecodable files - the #114 lesson), archive the original outside MEDIA_ROOT, then create the ItemImage row atomically. A failed step deletes the servable file so a row can never point at a broken path and no orphan file is left behind. No search auto-reindex: sync is manual everywhere in this system and uploads follow suit (the manuscript viewer is request-time, so uploads show there immediately). Also: GET item-images/{id}/original/ streams the archived preservation original; cleanup_stale_uploads management command (run by hand on the API container); task progress meta matches the search tasks so the frontend poller is reusable; session GET degrades instead of 500ing when the result backend is unreachable. Infra notes: Dockerfile now installs libvips-tools (worker-side conversion); config/test.env documents the two SIPI addresses (browser-facing IIIF_HOST vs worker-facing UPLOADS_SIPI_BASE_URL, which must reach SIPI over the compose network); .gitignore's unanchored `uploads` runtime pattern also matched apps/uploads - negated for the app source. The production compose must mount ./storage into the celery service (tracked in the infrastructure repo). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eleted (#72) Django's FileField never deletes the underlying file on row delete, and the uploaded original is tracked only by a path string — so deleting an ItemImage left both the served JP2 (storage/media) and the archived original (storage/originals) orphaned on disk. A post_delete signal now removes both, for every delete path (management API, backoffice edit dialog, and the ItemPart cascade), deferred to transaction.on_commit so a rolled-back delete keeps the files. Guards: - keeps a served file if any surviving ItemImage still references it (ItemImage.image has no unique constraint → paths can be shared); - path-containment check so a crafted/legacy `..` value can't delete outside the media/originals roots; - best-effort (never raises — the row is already gone), prunes emptied dirs. Verified live: upload → both files on disk → DELETE → both files and the row gone. New unit tests cover cascade, shared-path retention, traversal refusal, missing-file no-op, and the API path. Known follow-up: SIPI may serve a stale cached tile for a just-deleted path until its cache ages out or the container is recreated (existing landmine). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An unwritable storage directory (e.g. storage/originals recreated on the host with the wrong owner) surfaced only at the END of the pipeline as a bare "[Errno 13] Permission denied: …/originals/uploads/item-part-N" — after the editor had already uploaded and converted the whole file. - create_session now verifies, before accepting any byte, that the upload temp dir, the media destination and the originals archive folder are creatable/writable by the service user; failures return 503 with an operator-actionable message naming the offending directory. - The check targets the DEEPEST EXISTING ancestor of each actual target path (mkdir -p semantics), so a read-only corpus root does not block uploads whose subfolder tree is already writable. - _archive_original wraps PermissionError with the same actionable message for perms broken mid-flight; archive_folder() is now shared between preflight and ingest so both check/use the same directory. - schema.yaml documents the 503; tests cover locked originals, an uncreatable tmp root, and the read-only-media-root-with-writable- subfolder case. Dev note: container user is uid 999 (archetype); anything under storage/ that the pipeline writes must be writable by it — creating dirs on the host with default umask (775, uid 1000) breaks uploads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A mid-upload browser reload loses the client queue but not the server session, which kept its chunks in uploads_tmp AND squatted on the destination path — every retry then failed with 409 "Another upload session is already targeting …" until stale-cleanup (7 days). create_session now resolves an active-session collision by intent: - same owner + same declared size → hand the interrupted session back (HTTP 200, same id); the client resumes from missing_chunks, so a reload after N chunks re-uploads zero bytes. Locus/tags are refreshed from the retry. pending/uploading only — assembled/processing stay protected. - same owner + different size → the user re-picked a different file: the stale attempt is aborted (chunks deleted) and a fresh session created. - different owner, or already assembled/processing → 409 as before. 409 bodies now carry a machine-readable `code` (`destination_exists` vs `session_active`) so clients can distinguish a true duplicate from a transient hold instead of pattern-matching messages. Verified live against the real stuck session from the field report: create → 200 same id, missing_chunks [], finalize → complete without re-uploading any of the 184 MB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ons (#72) cleanup_stale_uploads only walked UploadSession rows, so a temp dir whose session row was deleted (abort/supersede, or a failed rmtree) became an orphan no query could ever see — leaking disk forever (found 300 MB of assembled.jpg orphans in dev). Add a second sweep over UPLOADS_TMP_DIR: remove any directory whose UUID isn't a live session and whose mtime is older than the threshold (the age check avoids racing a just-created dir). The command now reports both counts; cleanup_stale_sessions returns {'sessions', 'orphans'}. Verified live: --days 0 cleared 3 orphan dirs (302M -> 4K). Tests cover old orphan reaped / recent orphan kept / session-backed dir untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The create-or-resume endpoint hands every tab of the owner the SAME session, so two tabs can legally race on one chunk index or finalize together. With a shared temp path the loser's replace() raised an uncaught FileNotFoundError (a 500 mid-upload, observed in two-tab testing) even though the upload itself went on to succeed. - receive_chunk: unique per-request temp file + atomic replace (duplicate sends carry identical bytes, so last-writer-wins is safe); row-lock the received_chunks update so parallel chunks cannot lose an index and a session claimed by finalize is never flipped back to 'uploading' underneath the ingest pipeline - finalize_session: assemble into a unique temp file, then an atomic status compare-and-swap picks exactly one winner to commit the assembled file, sweep the chunks and dispatch ingest; raced callers get a controlled 409 instead of a 500 - guarded FAILED update so a raced finalize cannot clobber the winner's state The new tests reproduce the two-tab races deterministically (re-entrant stream, stale-snapshot finalize) and fail against the previous implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| body: dict[str, str] = {"detail": str(exc)} | ||
| if exc.code: | ||
| body["code"] = exc.code | ||
| return Response(body, status=exc.status_code) |
lanterno
reviewed
Jul 26, 2026
Comment on lines
+215
to
+231
| # Technical metadata captured by the upload pipeline (apps.uploads). All | ||
| # nullable/blank: the migrated corpus predates these and stays untouched. | ||
| width = models.PositiveIntegerField(null=True, blank=True) | ||
| height = models.PositiveIntegerField(null=True, blank=True) | ||
| source_format = models.CharField(max_length=16, blank=True, default="") | ||
| size_bytes = models.BigIntegerField(null=True, blank=True) | ||
| checksum_sha256 = models.CharField(max_length=64, blank=True, default="") | ||
| # Relative path of the archived upload under UPLOADS_ORIGINALS_DIR (not | ||
| # MEDIA_ROOT — originals must never be SIPI-servable). Blank for migrated | ||
| # rows and for uploads whose served .jp2 IS the original bytes. | ||
| original_path = models.CharField(max_length=255, blank=True, default="") | ||
| exif = models.JSONField(null=True, blank=True) | ||
| uploaded_by = models.ForeignKey( | ||
| settings.AUTH_USER_MODEL, null=True, blank=True, on_delete=models.SET_NULL, related_name="uploaded_images" | ||
| ) | ||
| created = models.DateTimeField(null=True, blank=True, auto_now_add=True) | ||
| modified = models.DateTimeField(null=True, blank=True, auto_now=True) |
Collaborator
There was a problem hiding this comment.
most of the images won't have these fields since they'll already be on the image server. so, it doesn't make sense that we store them here. the role of this model is just to hold a reference to the image on the SIPI server while the SIPI server (image server) would be responsible for anything related to the technical image details.
Collaborator
There was a problem hiding this comment.
Maybe leave this PR open for now, and explore what we can do with a direct connection to SIPI. can we upload images there directly? does the SIPI server allow for direct image uploads and does it support resumability?
Contributor
Author
There was a problem hiding this comment.
We can do direct connect to SIPI and upload images directly, it won't be resumable, which would be an issue especially for uploading images of multiple GBs (if I remember correctly from one of the meetings, I heard that a single image can reach up to 5GB).
This will also introduce orphan images, as the directly uploaded images will have no row in the ItemImage table.
I've been exploring some options, especially tus (tus.io) that seems to be the most used for resumable file uploads (along with uppy.io UI).
There's tusd, written in Go, it'll run as its own HTTP upload server in a separate container. It seems to be the standard, and specifically recommended for handling file uploads in the GBs.
There's also drf-tus, which is maintained (last release Jan 2026), but it doesn't declare Python 3.14 support, and every chunk would go through the Python stack, which wouldn't be ideal for GB files. So tusd seems to be a better option.
I lean towards tusd, mainly for its ability to handle big sizes. I'd like your opinion.
Some extra info worth noting:
SIPI has conversion to jp2, but it doesn't indicate that it's lossless and it's more prone to fall into OOM error during the conversion of big files, unlike vips' approach (vips streams instead of loading the whole image into memory).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The backend half of image upload (#72). Until now every
ItemImagearrived via the v2 bulk migration - there was no way to add one fromthe app, and the backoffice edit-save was broken (DRF mapped the IIIF field to a
binary
ImageFieldthat 400'd the JSON path it was sent). This adds superuser-only,chunked, resumable uploads targeted at an ItemPart, normalized to lossless JP2 before
SIPI ever serves them.
Key changes
apps/uploads:UploadSessionmodel + transport-only endpoints under/api/v1/uploads/(create/resume · chunk PUT · finalize with sha256+size verify ·status w/ Celery progress · delete · download-original). Orchestration in
services.py; Celeryingest_uploadconverts viavips(Pillow fallback),smoke-tests a real SIPI tile before creating the row, and archives the original
outside
MEDIA_ROOT. A failed step deletes partial output — a row can never pointat an unservable file.
ItemImagemetadata (nullable — migrated corpus untouched): dimensions, format,bytes, sha256, original_path, exif, uploader, timestamps. EditEvent auditing on.
imageis now a path-string-only field that rejectsraw bytes, so all bytes must go through the converting pipeline.
libvips-toolsin the Dockerfile;apps/<app>/schema.yamlupdated;architecture-boundary entry
uploads → {common, manuscripts, search};cleanup_stale_uploadscommand (run by hand — a backoffice "Trash" button is theplanned invoker, no cron).
Notes for reviewers
request-time so uploads show immediately). Frontend shows a one-shot reindex nudge.
0023withmain's
0023_msdescarea.Testing
Local CI-equivalent green: pytest, mypy, ruff (check+format),
check-architecture,coverage ≥55%. Pipeline proven end-to-end (upload→convert→tile→row→download, resume,
delete-removes-files, 503 preflight) and on the full infrastructure stack.
Part of #72 — pairs with the frontend
feat-72/image-upload-uiand infrastructurefeat-72/uploads-prod-enablementPRs.