fix: keep a moved legacy item's row pointing at the object that exists by GianniCarlo · Pull Request #47 · TortugaPower/bookplayer-api

GianniCarlo · 2026-09-15T16:29:17Z

The bug

A legacy item — one with source_path IS NULL — is read back from ${prefix}/${key}, so its S3 object is its display key. Moving one therefore has to relocate the object as well, which processMovedFiles does.

It treated that relocation as best effort:

S3Service.moveFile catches and return null
StorageService.moveFile catches and return null
processMovedFiles had if (isMoved) { updateBySourcePath(...) } — no else, no throw

So a failed copy still committed the key rewrite. The row ends up naming a key that holds nothing, while the bytes stay orphaned under the pre-move path.

Why nobody noticed

The failure is invisible in all three places you would look:

Not in CloudWatch. Both catch blocks call _logger.log({...}) with no level argument. LoggerService.log defaults to 'info', and prod runs LOG_LEVEL=warn, so the error is dropped before it leaves the container.
Not in the audit log. Nothing throws, so the request returns 200 and sync_operations records the move as applied / 200.
Not on the client. It sees success.

Only a diff of the DB keys against a bucket listing finds it.

Confirmed in production. One PRO account had 146 active books marked synced = true with no object at their key; 143 still had their bytes sitting at the pre-move path (Bluey 127, James May 6, Terminator 6, Breaking Bad 1, Yoga 3). The other 3 were unrelated zero-byte phantoms.

Why not just throw

That was the first instinct, and it is worse. processMovedFiles runs inside the caller's transaction, so throwing rolls the key rewrite back — but the items relocated earlier in the same batch are already at their new S3 keys, and the rollback strips the source_path that names them. You trade one orphan for several, pointing the other way.

The fix

Record reality. On a failed relocation, pin source_path to the pre-move key, so the row names where the bytes actually are. The move itself still succeeds — it is a display-path change — and the item stays playable from its legacy location. A later move is a no-op for storage, since the item now has a source_path.

Also in this change:

both relocation failures log at 'error', so they survive production log filtering, plus a line in processMovedFiles carrying id_user / oldKey / newKey
the two Promise<boolean> paths return false instead of null
dropped the dead suffix branch, which sat inside an if (type === BOOK) and so could only ever be ''

Tests

Two cases added to the existing move — S3 side effects (processMovedFiles) block:

a failed relocation still commits the move and pins source_path to the pre-move key
when one child of a moved folder fails, the sibling that did relocate keeps the timestamped source_path naming its new object — the case that rules out rolling back

Follow-ups, not in this PR

The 143 existing orphaned rows need a data patch setting source_path to the old path. Being scoped separately.
S3Service.deleteFile has the same swallow-and-log-at-info shape. Its failure only leaves a paid-for orphan rather than a broken row, so it is lower severity, but it should get the same treatment.
The root cause of the original relocation failures is unknowable — those logs were dropped. This PR is what makes the next occurrence visible.

🤖 Generated with Claude Code

A legacy item (source_path IS NULL) is read back from `${prefix}/${key}`, so moving one has to relocate its S3 object too. processMovedFiles did that, but treated the relocation as best effort: S3Service.moveFile and StorageService.moveFile both swallowed the error and returned null, and the `if (isMoved)` guard had no else. A failed copy therefore still committed the key rewrite, leaving the row naming a key that holds nothing while the bytes stayed orphaned under the pre-move path. Nothing surfaced it. Both catch blocks logged through LoggerService.log with no level, which defaults to 'info' and is dropped by the production LOG_LEVEL of 'warn'; the request still returned 200, so sync_operations recorded the move as applied; and the client saw success. Only a diff of the DB keys against a bucket listing finds it. Rolling the rewrite back on failure would be worse: items relocated earlier in the same batch are already at their new keys, and the rollback would strip the source_path naming them. So record reality instead -- on failure pin source_path to the pre-move key. The move still succeeds, since it is a display-path change, and the item stays playable from its legacy location. Also log both relocation failures at 'error' so they survive production log filtering, return false rather than null from the two Promise<boolean> paths, and drop the dead suffix branch that could only ever be ''.

github-actions · 2026-09-15T16:32:48Z

✅ Claude PR Review — `PASS`

Narrow, well-targeted fix: a failed legacy-item relocation no longer commits a key rewrite that points the row at a nonexistent object. processMovedFiles now probes S3 and pins source_path to whichever key actually holds the bytes, fileExists becomes honestly tri-state (403/5xx → null instead of a false "absent"), both relocation failures log at 'error' so they clear the production LOG_LEVEL=warn, and stripStoragePrefix keeps the legacy email prefix out of CloudWatch. I traced the read path (${prefix}/${source_path || key} at LibraryService:165/245/318/472/796/923/1018) and confirmed pinning old_key keeps the item playable; deleteFile is single-object so a pinned object under an old folder prefix is not swept; and the widened boolean | null return is safe for the two existing callers (=== true and a truthiness check), since 403 was already falsy. No routes, middleware, or auth surfaces change; updateBySourcePath stays scoped to user_id + key + active and the storage prefix is still derived server-side from the authenticated user, so there is no IDOR or auth-coverage regression. Earlier findings #1, #2 and #3 look addressed; remaining comments are advisory — the in-transaction HEAD probes still have no timeout, copied is computed but not returned, and the give-up branch leaves no durable record.

Findings: 3 info

Previously raised

Finding	Status
`src/services/S3Service.ts:63` (warn) ⚠︎ moved	✅ verified fixed in `fc2e185`
`src/services/LibraryService.ts:1310` (info)	✅ verified fixed in `fc2e185`
`src/services/StorageService.ts:25` (info)	✅ verified fixed in `fc2e185`

_{Model claude-opus-5 · run log · 2 new · 1 carried over · 3 verified closed · 1 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.}

- Strip the per-user storage prefix from the keys logged by the two moveFile catches. That prefix is the account's email for legacy accounts, which is exactly the population this path serves, and LoggerService redacts only password/token/secret/authorization — so raising these to 'error' had started emitting addresses to CloudWatch. Added stripStoragePrefix to utils rather than duplicating the split. - Only pin source_path when the object is actually still at the old key. A relocation can also fail because there was nothing to copy; pinning then records a path holding nothing and, since the guard skips items that already have a source_path, no later move would retry. fileExists returns null when the probe itself fails, so only a definitive false suppresses the pin. - Track whether the copy landed in S3Service.moveFile and log it with error.name, so "copy failed" and "copy succeeded, delete failed" — which both surface as false — are distinguishable in the logs. - Rename the two log origins to ClassName.methodName per CLAUDE.md. Tests: the two failure cases now assert against a present source object, plus a new case covering the absent-source branch.

- Probe the target key before giving up on a failed relocation. A `false` from fileExists does not mean the bytes are nowhere: moveFile is copy-then-delete, so a delete that landed on S3 but lost its response reports failure with the object already at the target and gone from the source, and fileExists reports 403 as false too. Round 1 would have left those rows with a rewritten key and a null source_path — the exact breakage this PR exists to prevent. Now the source is pinned when it is still there, the target when the move really completed, and only "found at neither" leaves source_path null. - Widen S3Service.fileExists and StorageService.fileExists to Promise<boolean | null> so the tri-state this logic depends on is part of the contract, not an undocumented runtime detail. LibraryService:320 already relied on it. Tests: the lost-delete-response case, the indeterminate-probe case, and a new src/__tests__/utils/stripStoragePrefix.test.ts pinning the edges of the helper that keeps addresses out of CloudWatch.

Return null, not false, for a 403 from S3Service.fileExists. A permission or KMS failure hits both the source and the target probe identically, so the previous `false` let processMovedFiles satisfy `sourceStillThere === false && targetLanded !== true` and conclude the bytes were nowhere — leaving source_path null against an already rewritten key, in the one case where the correct pin (old_key) was available all along. S3 masks a missing key as 403 only when the caller lacks s3:ListBucket, and this role holds it (getDirectoryContent and calculateFolderSize both call ListObjectsV2), so a 403 here is a real permission anomaly rather than a 404 in disguise. The other two callers are unaffected: one tests `=== true`, the other is a truthy check, and null is falsy for both. Also strips the storage prefix from the keys this method logs, for the same reason the moveFile catches do. Tests: a new S3ServiceFileExists suite pinning 200/404/403/other to true/false/null/null, plus that the logged key carries no address.

github-actions · 2026-09-15T16:55:00Z

+              // lost its response (bytes at the target, source already gone).
+              // fileExists also reports 403 as false, so a false is "could not
+              // find it", not "it is not there". Probe before concluding.
+              const sourceStillThere = await this._storage.fileExists({


🔵 INFO — These probes run inside the caller's transaction (trx is threaded straight through to updateBySourcePath), so each failed item now adds one or two synchronous headObject round trips while holding the pooled connection and the row locks taken by the key rewrite. On a large folder move where S3 is the thing that is unhealthy — precisely when this path fires — the probe latency is also the worst, and the failure mode escalates from "one broken row" to "a sync transaction pinned open across many S3 timeouts".

The S3 client has no explicit timeout configured here, so consider a short requestHandler timeout on the client used for probes, or capping the number of items probed per batch and pinning the rest to old_key (the common-case answer) without a probe.

Flagging this one for a maintainer rather than picking an option.

Worth noting the framing: S3 calls already ran inside this transaction before the PR — moveFile is a copy plus a delete per item, called from the same place with the same trx. The probes add one or two HEADs on the failure path only, so this is roughly a 1.5-2x increase in round trips on an already-pathological path, not a new class of exposure. That is a reason to weigh the options rather than to rush one in.

The suggestions differ materially:

A requestHandler timeout on the S3 client is global — the same client serves getObjectStream, so a short timeout risks breaking large-file streaming. It would need a separate client for probes.

Capping probes per batch and pinning the rest to old_key unprobed trades away the correctness this PR just established: the unprobed items go back to being pinned blind, which is exactly what rounds 2 and 3 were about.

Accepting it leaves a pre-existing characteristic in place.

Different trade-offs on different axes, so leaving it for the maintainers to choose as a follow-up.

Reported again on the newest commit, worded differently — the current wording is:

Still open from the previous push: these probes run inside the caller's transaction (trx is threaded straight to updateBySourcePath), so each failed item adds one or two synchronous headObject round trips while the pooled connection and the row locks from the key rewrite are held. This path fires precisely when S3 is unhealthy, which is also when probe latency is worst, and the S3 client is built with no requestHandler timeout (new S3({ region }) in S3Service), so the default SDK retry/timeout budget applies.

Concrete options: configure a short requestHandler connect/socket timeout on the client used for probes, or cap the number of probed items per batch and pin the remainder to old_key (the common-case answer) without probing.

- Log the wider indeterminate branch of S3Service.fileExists at 'warn'. Round 3 raised only the 403 sibling, leaving 5xx, timeouts and SDK failures at 'info' — dropped by the production LOG_LEVEL, even though they drive the same pin decision. Added error.name while there. - Give StorageService.fileExists' own catch the same treatment its moveFile sibling got in round 1: prefix-stripped key instead of the raw params object, ClassName.methodName origin, and 'warn'. - Log the relocation decision, not just its inputs: foundNothing and targetIndeterminate let a remediation sweep separate a confirmed phantom (both probes 404) from one whose target probe was merely indeterminate.

github-actions · 2026-09-15T17:02:12Z

+    // never landed (bytes only at sourceKey) or the copy landed and the delete
+    // did not (bytes at both). `copied` tells them apart in the log — the
+    // caller only sees false either way.
+    let copied = false;


🔵 INFO — copied is exactly the signal the caller needs, but it only reaches the log — moveFile still collapses to false, so processMovedFiles re-derives it with up to two HEAD requests inside the transaction. Returning it would remove those probes entirely on the common path:

async moveFile(sourceKey, targetKey): Promise<{ moved: boolean; copied: boolean }>

It also matters for cleanup: in the copy-landed/delete-failed shape the caller now pins source_path = old_key (because the source probe says true), which leaves the target duplicate as a permanent paid-for orphan that nothing will ever delete — and with source_path set, no later move retries. With copied visible, that case could delete the target copy or at least be recorded distinctly. Not blocking; the current behavior is still strictly better than before.

github-actions · 2026-09-15T17:02:13Z

-                trx,
+                'error',
              );
+              if (foundNothing) continue;


🔵 INFO — The give-up branch leaves the row in the exact state the PR describes as invisible-by-default: source_path NULL, request still 200, and recordSyncOperation derives outcome from res.statusCode (src/api/middlewares/recordSyncOperation.ts:147), so sync_operations will again record applied / 200. The only trace is the CloudWatch line above, which is subject to log retention — the same gap that made the original 146-item case discoverable only by diffing DB keys against a bucket listing.

Since the durable audit table already exists, consider making processMovedFiles return the degraded item count and having the controller stash it on res.locals for the audit middleware to persist (or record the unresolved item ids directly). That would let the planned remediation sweep query Postgres instead of depending on log retention.

GianniCarlo deployed to reviewer September 15, 2026 16:29 — with GitHub Actions Active