feat(library): upload books to S3 in parts, confirmed by the server - #50
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
✅ Claude PR Review —
|
A single presigned PUT capped every book at 5 GiB, restarted a stalled upload from byte zero, and was frozen into the client's queue even though the task-role credentials that sign it expire within hours. Clients also confirmed `synced:true` when S3 had rejected the PUT, which is how rows end up claiming a backup with nothing in S3. Add stateless multipart routes under /v1/library/upload (start, parts, complete, abort). The server derives every key from the caller's own row, reads S3's part list instead of trusting the client, and `complete` is the only thing that marks a multipart upload synced — including a media-server book's external resources, so the single-PUT pipe (POST /external_set) is removed. Part URLs are signed without the SDK's empty-body checksum. `synced` now means "the file is in S3" on every tier: POST / ignores a confirmation for a book with no object and still answers 200, so older clients stop creating phantom rows without being pushed into a retry loop. Deleting a book aborts its open uploads, a book deleted while S3 assembles its upload no longer leaves the object behind, and books over 5 GiB are deleted without the support copy a single CopyObject can't make. Re-uploading a legacy row (no source_path) now targets the key it reads from instead of a path never written back. Contract and client error handling: docs/multipart-uploads.md.
- Normalise `synced` to a real boolean before the synced guard: Postgres casts 'true', 't', 1… to true on its own, which slipped past the `=== true` check. A value that is neither is dropped, not written. - Read only an S3 `NoSuchUpload` (or a bare 404 with no error name) as a vanished upload, so a NoSuchBucket from a misconfigured bucket surfaces as a 500 instead of sending clients into restarts. - Cap `partNumbers` at 10,000 entries in the schema, the most distinct parts an upload can have; duplicates below that are still collapsed before the service's 32-URL cap.
`complete` trusted the client's partCount. A count that is too low while every counted part exists (a client rounding fileSize/partSize down, say) passed every check, so S3 assembled a truncated file and the row was marked synced — the "synced but not backed up" state this change exists to end. `complete` now also takes the fileSize the client read from disk, and answers invalid_parts before S3 assembles anything unless the parts add up to exactly that size and their count is ceil(fileSize / partSize), which also catches an empty trailing part.
Stop accepting `source_path` from clients on POST /. putObject assigns it, and it now decides which S3 object complete, the delete-time abort and downloads touch; passing a client value straight into the row would let a buggy or crafted client point one book at another book's bytes (and delete them with it). No iOS, Android or web client sends it, so this changes nothing for them.
Keep each comment next to the declaration it documents: CLIENT_READONLY_FIELDS had landed between the isTrue comment block and isTrue itself.
Tell fileExists callers to check `=== false` for "missing", never `!exists`: a 403 answers null (unknown). The Promise<boolean | null> signatures themselves arrived with #47.
6afa0e5 to
8a033f6
Compare
Multipart lifted the single-PUT 5 GiB ceiling all the way to S3's 5 TiB object limit, with no per-user quota, so one crafted client could store a multi-terabyte "book". Real ones are far smaller: the longest known audiobook (Wind and Truth, 62 h 48 min) is ~1.7 GiB at 64 kbps and ~6.7 GiB even at 256 kbps. Enforce a 10 GiB ceiling (invalid_request, which clients treat as "don't retry") in three places, because the server keeps no state: - start, before an upload exists; - complete, which can't trust start's fileSize — and since the parts must add up to exactly fileSize, nothing over the ceiling becomes an object. The upload is aborted there, freeing its parts now instead of in 7 days; - POST /parts, refusing part numbers above 2,048 (10 GiB in 5 MiB parts).
- Log S3 keys without the storage prefix in the new multipart and delete paths (stripStoragePrefix, as fileExists and moveFile already do): for legacy accounts the prefix is the user's email. listMultipartUploads no longer logs its prefix at all. Failures log at error, the no-support-copy delete at warn: at the production LOG_LEVEL info-level failures vanish. - PUT / creates rows unsynced whatever `synced` the client sends: a new row has no file, and only an upload's confirmation or `complete` may mark it. - Document that GET /keys lists books whose file is in S3, so LITE books are absent by design, and that clients run the missing-uploads pass only on PRO.
- Keep a book to one open multipart upload. start now asks S3 which uploads are open for exactly the book's key and aborts them before opening a new one (or answering "exists"). A client calls start only without an uploadId — fresh upload, lost start response, restart — so anything still open is unreachable, and a client retrying start could otherwise pile up parts billed for 7 days each. S3 holds the state; the server still keeps none. A failed listing doesn't block the upload. - Skip the synced guard's S3 HEAD for a row that is already synced: dropping the confirmation would change nothing, and older clients repeat it often.
start keeps a book to one open upload, so two devices uploading the same book at once would cancel each other. That is deliberate — the importing device uploads, every other device downloads — so say it in the contract rather than leave clients to discover it.
| const { key } = await this.resolveTarget(user, uuid); | ||
| const urls: PartUrl[] = []; | ||
| for (const partNumber of unique.sort((a, b) => a - b)) { | ||
| const signed = await this._storage.getPresignedPartUrl(key, uploadId, partNumber); |
There was a problem hiding this comment.
🔵 INFO — Part URLs don't limit how many bytes a part can hold. Each part URL signs only the host header, so a PRO caller can PUT up to 5 GiB (S3's per-part limit) to every part number up to 2,048, for any number of uploadIds it opens across its books. complete rejects anything over 10 GiB, but parts that are never completed stay stored and billed until the 7-day lifecycle rule removes them. That could be terabytes per account. The old single presigned PUT stopped at 5 GiB per object.
Optional hardening: sign ContentLength into UploadPartCommand so S3 refuses a body of any other size. The client already knows each part's exact size, since partSize is fixed and the last part is the remainder. This requires content-length to be a signed header (confirm the presigner keeps it). The API would have to accept partSize/fileSize in POST /parts, or recompute them from a value sent at start. At minimum, add an alarm on incomplete-multipart storage. This isn't blocking: exploiting it needs a paid account, and the lifecycle rule caps how long the storage lasts.
There was a problem hiding this comment.
Considered and accepted for now. The server is stateless, so signing ContentLength would only bind a size the client itself reports to POST /parts. That constrains a buggy client, not a hostile one. The realistic runaway case (a client retrying start) is already closed by keeping one open upload per book. What's left needs a paid PRO account, is capped at 2,048 parts per book, and is cleared by the 7-day abort-incomplete-multipart-uploads rule. An alarm on incomplete-multipart bytes is a good follow-up outside this PR.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
Still open: part URLs don't limit how many bytes a part can hold. getPresignedPartUrl signs only host, so a PRO caller can PUT up to 5 GiB to every part number up to 2,048, across as many uploadIds as it opens. Parts that are never completed stay billed until the 7-day lifecycle rule removes them. Optional hardening: sign ContentLength into UploadPartCommand (the client knows each part's exact size from partSize/fileSize), or at minimum add an alarm on incomplete-multipart storage. Not blocking: exploiting it needs a paid account, and the lifecycle rule caps how long the storage lasts.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
Still open: part URLs don't limit how many bytes a part can hold. getPresignedPartUrl signs only the host header, so a PRO caller can PUT up to 5 GiB to each part number up to 2,048, for any number of uploadIds it opens. complete rejects anything over 10 GiB, but parts that are never completed stay stored and billed until the 7-day lifecycle rule removes them.
Optional hardening: sign ContentLength into UploadPartCommand so S3 refuses a body of any other size. The client knows each part's exact size (partSize, with the last part being the remainder). Or at least add an alarm on incomplete-multipart storage. Not blocking: exploiting it needs a paid account, and the lifecycle rule caps how long the storage lasts.
markItemSynced set sync_status = 'downloaded' on every active external
resource of the book. A book can also be linked to Hardcover, which has no
file: iOS keeps a link marker in that row's sync_status and Android keeps
its Hardcover reading state there ('library', 'reading', 'read'), so one
multipart upload would have overwritten it. Production already holds a
Hardcover row, so the combination isn't hypothetical.
Scope the update to MEDIA_SERVER_PROVIDERS (jellyfin, audiobookshelf), the
only providers a book's file comes from.
iOS (JSONSerialization of a Swift Bool) and Android (Gson map through Retrofit) both send synced as a JSON boolean, so the Postgres-spelling normalisation only defended against clients we don't support. Drop it and check synced === true, as before.
… checks
Every limit on what a client may send now lives in a schema, and the
services only check requests against S3 and the database.
- POST / and PUT / get schemas (validation/libraryItem.ts) matching what
iOS and Android send: every metadata field optional, numbers not forced
to integers, relativePath optional on POST (Android updates by uuid).
Unknown keys are stripped, which replaces the hand-written rules keeping
`source_path` (and `synced` on PUT) out of client writes. Checked against
two days of production sync_operations: every key and JSON type matches.
- The multipart schemas take over the service's checks: partSize range, the
10 GiB book ceiling (start and complete), part numbers up to 2,048 and 32
URLs per request. Gone with them: part-number de-duplication, the
10,000-part check (unreachable under the ceiling), the abort on an
oversized complete, and uploadId trimming.
- validateBody answers `code: 'invalid_request'` with every 422, so clients
branch on the documented code for schema rejections too.
- Drop two checks for requests the apps never send: the updateObject early
return for a uuid-only `{ synced }` body, and reading a nameless 404 as
NoSuchUpload (the SDK always names it).
Let POST / and PUT / metadata fields be null as well as missing (.nullish() instead of .optional()). The parser behind these routes has always tolerated null — the numeric fields skip it, the text ones pass it through — and both apps retry a failed sync job forever, so a 422 on a null in a task queued by an older build would wedge that user's sync for good. Neither app sends null today (iOS omits nils, Gson drops them) and a sample of the full sync_operations history has none; this only keeps the schema from being stricter than the parser it guards. relativePath, uuid and synced still reject null: there it would write the row's key, uuid or synced flag as NULL.
| await tx('external_resources') | ||
| .update({ sync_status: 'downloaded' }) | ||
| .where({ library_item_id: libraryItemId, active: true }) | ||
| .whereIn('provider_name', MEDIA_SERVER_PROVIDERS); |
There was a problem hiding this comment.
🔵 INFO — This provider match is case-sensitive, but provider_name is never normalized. putExternalResourceSchema stores providerName as the client sent it (only .trim()), and MEDIA_SERVER_PROVIDERS is ['jellyfin', 'audiobookshelf']. If either app registers the link as Jellyfin or Audiobookshelf, complete marks the row synced but silently skips the downloaded update. That update is the only confirmation a media-server book's resources now get, because /external_set is gone. I couldn't confirm the exact strings the clients send, since this repo only has test fixtures. Either confirm they're lowercase, or match case-insensitively:
.whereRaw('lower(provider_name) = any(?)', [MEDIA_SERVER_PROVIDERS])You could also normalize providerName to lowercase in putExternalResourceSchema (.toLowerCase()).
There was a problem hiding this comment.
Confirmed lowercase on both clients and in the data:
- iOS
ExternalResource.ProviderNamehas raw valuesjellyfin,hardcoverandaudiobookshelf(Shared/CoreData/Backed-Models/ExternalResource+CoreDataClass.swift:33). - Android uses the same strings.
- Every active
external_resources.provider_namein production isaudiobookshelf,jellyfinorhardcover.
We validate against what the apps send, so no case folding.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
Still open: this provider match is case-sensitive, but provider_name is never normalized. putExternalResourceSchema only trims providerName, and MEDIA_SERVER_PROVIDERS is ['jellyfin', 'audiobookshelf']. If a client registers Jellyfin, complete marks the row synced but silently skips the downloaded update. With /external_set gone, that update is the only confirmation a media-server book's resources get.
Either confirm the clients send lowercase names, or match case-insensitively:
.whereRaw('lower(provider_name) = any(?)', [MEDIA_SERVER_PROVIDERS])You could also add .toLowerCase() to providerName in putExternalResourceSchema.
Move the machine-readable code on 4xx bodies from `code` to `error`, the key the passkey routes already use and that iOS's ErrorResponse decodes into networkErrorWithCode. One convention across the API, and clients can branch on the multipart and validation codes with no model change.
This branch was successfully deployed
1 participant
Why
Books go to S3 as one presigned
PUT, and that breaks in three ways:PutObjectcan't exceed it. The 2026-09-20 inventory has 0 of 969,604 objects above 5 GiB. That's a ceiling, not an absence of big books.synced:trueeven when S3 rejected the PUT, which leaves rows that claim a backup with nothing in S3.What
Multipart upload routes:
/v1/library/upload/{start, parts (POST + GET), complete, abort}, PRO only.uploadId, the key always comes from the caller's own row (the client never sends a key), and S3's part list is the source of truth for resuming and completing.completeis the only thing that marks a multipart uploadsynced. For a media-server book it also marks the external resourcesdownloaded.codes for clients to act on:item_not_found,upload_not_found,parts_missing(with the missing part numbers),invalid_parts,invalid_request.docs/multipart-uploads.md.syncednow means "the file is in S3", on every tier.POST /ignoressynced:truefor a book whose object is missing, and still answers 200 so older clients don't retry forever. That also covers LITE clients, which readurl: nullas "already stored". A LITE account that was once PRO still confirms the files it uploaded then.POST /external_setis removed. It isn't used in production, and the media-server pipe now goes through/upload/*.Delete changes:
deleted_support copy, because a singleCopyObjectcan't make it. Any other copy failure behaves as before.Other fixes:
S3Service.fileExiststreats a 403 as unknown. The task role hass3:ListBucket, so a missing key answers 404.source_path) now targets the key it's read from. It used to go to a timestamped path that was never written back, which orphaned the bytes.upload_start,upload_completeandupload_abortare logged. Part-URL requests aren't.Deploy notes
abort-incomplete-multipart-uploads(7 days);S3Accessgaineds3:ListMultipartUploadParts,s3:AbortMultipartUploadands3:ListBucketMultipartUploads. Without these, everycompletewould be refused. They're checked withsimulate-principal-policy.StreamFileUploadProcessor) calls the removedexternal_setuntil it adopts these routes. It isn't in production.UPLOAD_PART_URL_TTL_SECONDSis an optional, development-only setting for testing expired part URLs. It's ignored in production.Testing
yarn test: 360/360 on a throwaway Postgres 17. New suites cover the service, S3 wire behaviour (the real presigner),markItemSynced, the guards and delete paths against real rows, and the controller's error mapping.S3Serviceagainst a scratch prefix inbookplayer-library, then emptied:curlPUTs of the parts succeed;listPartsreturns the right sizes and ETags;completeproduces an exactly 7 MiB object;NoSuchUploadaftercompleteand afterabort, and a second abort is still a success.