feat(library): stable error codes on the legacy library routes - #51
Conversation
Both apps retry a failed sync task forever, and every legacy library route
answered any failure 400 { message }: a failed DB read, an item deleted on
another device and a request that can never succeed all looked the same.
Give the permanent failures a stable `error` code the apps can stop on and
report, and make the rest answer so that retrying is right.
- `not_subscribed` (400) and `tier_required` (403) from the subscription
middlewares.
- `item_not_found` (404) when no row, active or deleted, has the uuid the
request names. An item the user deleted answers success with nothing
changed instead (LibraryService.confirmDeleted), so a task for it no
longer wedges the queue. New partial index (uuid, user_id) WHERE
active = false keeps that check from scanning library_items.
- `uuid_conflict` (409) on PUT / when the uuid belongs to another item type.
- Removals of something already gone answer success: bookmark delete,
external unlink, folder_in_out. A bookmark delete no longer inserts a row
(the upsert created a bookmark the user had deleted, as active).
- A failed DB read is a 500 (LibraryLookupError), never a "not found".
- moveLibraryObjectByUuid no longer moves an item to the root when its
named destination folder is missing (regressed in the April refactor).
- The audit log records one account-level rejection per user, job type and
code every 10 minutes: lapsed clients repeat it every 5 seconds.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🟡 Claude PR Review —
|
| Finding | Status |
|---|---|
src/services/LibraryService.ts:1049 (warn) |
✅ verified fixed in 031ad3d |
src/api/middlewares/subscription.ts:65 (warn) ⚠︎ moved |
✅ verified fixed in 031ad3d |
Model claude-opus-5-5 · run log · 0 new · 2 carried over · 2 verified closed · 2 re-worded on their own thread · 0 resolved · advisory (a human should still review). Findings are de-duplicated across pushes; an earlier finding closes only when the verification pass judges it against the current code — fixed, no longer applicable, accepted by a maintainer, or a duplicate of a finding reported on this push.
Declare library_items_key_index in the migration. Production has it (valid, and it serves hasDeletedItem's by-key lookup), but no migration created it, so fresh databases went without. CREATE INDEX CONCURRENTLY IF NOT EXISTS is a no-op where it exists; down leaves it alone.
| }); | ||
| if (!itemDB || !itemDB[0]) { | ||
| throw new Error('Invalid key'); | ||
| if (itemDB === null) throw new LibraryLookupError(); |
There was a problem hiding this comment.
🟡 WARN — PUT /bookmark has no body validation, so a request with no valid uuid and no key reaches getLibrary(user.id_user, undefined, …). There, path.replace(...) throws a TypeError. getLibrary catches it, logs it as a DB error and returns null, and this new line turns that into LibraryLookupError, so the client gets a 500 'Internal error'. Before this PR the same request got 400 'Invalid key'. That goes against the PR's own rule: a request that can never succeed is reported as a retryable server fault, and every retry writes an error-level log entry.
Fix: check the input before the lookup, e.g.
if (!isValidUUID(bookmark.uuid) && !bookmark.key) {
return res.status(422).json({ message: 'Invalid key', error: ApiErrorCode.INVALID_REQUEST });
}or add a validateBody schema to the route (a time number, plus a uuid or a key).
There was a problem hiding this comment.
Not changing this one: both apps always send key with a bookmark (iOS LibraryAPI.swift:161 "key": path, Android CoreProcessors.kt "key" to payload["relativePath"]), so a body with neither a valid uuid nor a key isn't something a shipped client produces; either answer is retried the same way.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
PUT /bookmark still has no body validation. The router registers it with only checkSubscription, requireCloudData. A request with no valid uuid and no key calls getLibrary(user.id_user, undefined, { exactly: true }). There, path.replace(...) throws a TypeError, which is caught and returned as null. This line then turns that into LibraryLookupError, so the client gets a 500 'Internal error' and every retry writes an error-level log entry. Deletes (active: false) take the same path. Before this PR the answer was 400 'Invalid key'.
Fix: check the input before the lookup, e.g.
if (!isValidUUID(bookmark.uuid) && !bookmark.key) {
return res.status(422).json({ message: 'Invalid key', error: 'invalid_request' });
}Or add a validateBody schema to the route: a numeric time, plus either a uuid or a key.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
Still present. PUT /bookmark has no body validation. A body with no valid uuid and no key calls getLibrary(user.id_user, undefined, { exactly: true }), where path.replace(...) throws. getLibrary then returns null, and this line turns that into LibraryLookupError: a 500 'Internal error', logged at error on every retry. Before this PR it was a 400. A request that can never succeed should get a coded 4xx.
Fix: check the input before the lookup, e.g. if (!isValidUUID(bookmark.uuid) && !bookmark.key) return res.status(422).json({ message: 'Invalid key', error: 'invalid_request' });. Or add a validateBody schema to the route.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
Still open. PUT /bookmark has no body validation. A body with no valid uuid and no key reaches getLibrary(user.id_user, undefined, { exactly: true }), where path.replace(/\/+$/, '') throws a TypeError. getLibrary catches it, logs it and returns null, and this line turns that into LibraryLookupError, so the client gets a 500 'Internal error' plus an error-level log entry on every retry. Before this PR it was a 400. That goes against the PR's own rule: a request that can never succeed is reported as a retryable server fault.
Fix: check the input before the lookup, e.g.
if (!isValidUUID(bookmark.uuid) && !bookmark.key) {
return res.status(422).json({ message: 'Invalid key', error: 'invalid_request' });
}or add a validateBody schema to the route (a numeric time, plus a uuid or a key).
Rebuild library_items_key_index when a failed concurrent build left it INVALID: IF NOT EXISTS would otherwise skip it for good. Only an invalid copy is dropped; production's valid one predates this migration.
| params.origin, | ||
| null, | ||
| trx, | ||
| const [originDB] = this.requireLookup( |
There was a problem hiding this comment.
🟡 WARN — LibraryController.moveLibraryObject sends { origin: '', destination: <uuid> } down the uuid path (isValidUUID(destination) && origin === ''). Here that becomes getLibraryByUuid(user.id_user, ''). library_items.uuid is a Postgres uuid column, so where uuid = '' fails with invalid input syntax for type uuid. The DB method returns null, and requireLookup now raises LibraryLookupError, which answers 500 and logs at error level. Before this PR, destructuring null gave a 400.
Moving the library root into a folder can never succeed, so under this PR's contract it should be a coded 4xx, not a retryable server fault.
Fix: reject it before any lookup, e.g. at the top of moveLibraryObjectByUuid:
if (!isValidUUID(params.origin)) {
throw new Error('Invalid origin'); // 400, or a coded 422
}Or drop the origin === '' branch from useUuids in the controller.
There was a problem hiding this comment.
Not changing this one: no shipped client sends an empty origin on a move. iOS sends the item's uuid as origin only when it's real, otherwise its relativePath (SyncJobScheduler.scheduleMoveItemJob), and Android always sends item.uuid (SyncingLibraryRepository.kt:171, 224). Either answer is retried the same way by today's apps.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
Still present. The controller sends { origin: '', destination: <uuid> } down the uuid path. Here that becomes getLibraryByUuid(user.id_user, ''). library_items.uuid is a Postgres uuid column, so where uuid = '' fails with an invalid-input error. The DB method returns null, and requireLookup raises LibraryLookupError: a 500 logged at error. Moving the library root can never succeed, so it should be a coded 4xx.
Fix: at the top of moveLibraryObjectByUuid, reject !isValidUUID(params.origin) with a 400/422. Or drop the origin === '' branch from useUuids in LibraryController.moveLibraryObject.
There was a problem hiding this comment.
Reported again on the newest commit, worded differently — the current wording is:
Still open. LibraryController.moveLibraryObject sends { origin: '', destination: <uuid> } down the uuid path (isValidUUID(destination) && origin === ''). Here that becomes getLibraryByUuid(user.id_user, '') against the Postgres uuid column, which fails with invalid input syntax for type uuid. The DB method returns null, and requireLookup raises LibraryLookupError: a 500 logged at error level. Before this PR it was a 400. Moving the library root can never succeed, so under this PR's contract it should be a 4xx, not a retryable server fault.
Fix: reject it before any lookup, e.g. at the top of moveLibraryObjectByUuid:
if (!isValidUUID(params.origin)) {
throw new Error('Invalid origin'); // 400, or a coded 422
}or drop the origin === '' branch from useUuids in the controller.
not_subscribed and tier_required come from apps retrying the same task every 5 seconds; each was a DB write that only bumped a counter, and RevenueCat already answers whether the account is subscribed. Replaces the per-process throttle.
- Only send not_subscribed / tier_required when RevenueCat confirmed the negative. When RC can't be reached the same 400/403 goes out without the code, so an outage never tells the apps to stop. isActive now labels that unconfirmed negative 'local' instead of 'rc'. - softDeleteExternalResource returns undefined for no match and keeps null for a failed query, so a failed unlink write is a retryable 500 instead of reading as already unlinked.
This branch was successfully deployed
1 participant
Why
Both apps retry a failed sync task forever. Every legacy library route answered any failure
400 { message }, so a failed DB read, an item deleted on another device and a request that can never succeed all looked the same to them. The audit log shows the result: 364 users stuck on "You are not subscribed" (7.7M requests in 14 days), ~43 on a move "Item not found", 20 on a bookmark "Invalid key", with no 5xx at all.This gives the permanent failures a stable
errorcode the apps can stop on and report (the key the passkey routes already use, which iOS decodes intonetworkErrorWithCode), and makes everything else answer so that retrying is right. Shipped apps are unaffected: they treat every failure the same and ignore the new key. Deploys together with #50.What changes
{message}{message, error: "not_subscribed"}{message}{…, error: "tier_required"}LibraryService.confirmDeleted){…, error: "item_not_found"}folder_in_outLibraryLookupError)PUT /with a uuid that belongs to another item type{message}{…, error: "uuid_conflict"}Also:
LibraryDB.deactivateBookmarkreplaces the upsert foractive: false; the upsert's insert created a bookmark the server never had, as active.moveLibraryObjectByUuidno longer moves an item to the root when its named destination folder is missing. The April refactor turned that case intodestinationDB?.key || ''. A deleted destination is now a no-op; a never-existing one isitem_not_found.destination: ""still means the root.not_subscribed/tier_requiredare the same stuck task retrying every 5 seconds (~550k upserts a day today), and RevenueCat answers the account's state.CLAUDE.mddocuments the codes.Migration
20260924120000_library_items_uuid_user_inactive: partial index(uuid, user_id) WHERE active = false. Without it, "did this user delete the item with this uuid?" is a parallel sequential scan of ~1.6M rows, on a path old clients hit every 5 seconds. Built withCREATE INDEX CONCURRENTLY(config.transaction = false), so the table stays writable; it drops any INVALID leftover from a failed build first, so a re-run rebuilds it. It also declareslibrary_items_key_index (key)withIF NOT EXISTS: production has it (valid, and it serves the by-key check) but no migration ever created it, so fresh databases went without. A no-op in production;downleaves it alone.Tests
LibraryServiceMissingItems.test.ts: deleted vs never-existed for both move paths (incl. no folder left behind by a path-mode no-op, never falling back to the root), external link/unlink, thumbnails, the 409, lookup failures,hasDeletedItem,deactivateBookmark.