From c6856b327f4712a6efdcafbf53634bb99d4edf1d Mon Sep 17 00:00:00 2001
From: Gianni Carlo <gcarlo89@hotmail.com>
Date: Tue, 15 Sep 2026 11:28:53 -0500
Subject: [PATCH 1/5] fix: keep a moved legacy item's row pointing at the
 object that exists

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 ''.
---
 .../LibraryServicePathMutations.test.ts       | 80 +++++++++++++++++++
 src/services/LibraryService.ts                | 43 +++++++---
 src/services/S3Service.ts                     | 17 ++--
 src/services/StorageService.ts                | 17 ++--
 4 files changed, 134 insertions(+), 23 deletions(-)

diff --git a/src/__tests__/services/LibraryServicePathMutations.test.ts b/src/__tests__/services/LibraryServicePathMutations.test.ts
index acc9d71..21b427a 100644
--- a/src/__tests__/services/LibraryServicePathMutations.test.ts
+++ b/src/__tests__/services/LibraryServicePathMutations.test.ts
@@ -328,6 +328,86 @@ describe('LibraryService — path-mutating flows (move / rename / folder_in_out)
 
       expect(moveFileMock).not.toHaveBeenCalled();
     });
+
+    it('pins source_path to the pre-move key when the relocation fails, so the row still names the object that exists', async () => {
+      const trx = getTestTransaction();
+      const user = await createTestUser(trx);
+      const book = await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: 'Legacy.m4b',
+        source_path: null,
+      });
+      await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: '0_FINISHED',
+        type: 0,
+      });
+      moveFileMock.mockImplementation(async () => false);
+
+      await service.moveLibraryObject(user as any, {
+        origin: 'Legacy.m4b',
+        destination: '0_FINISHED',
+      });
+
+      const bookAfter = await trx('library_items')
+        .where({ id_library_item: book.id_library_item })
+        .first();
+      // The move still commits — it is a display-path change — but the row now
+      // points at the object's real, un-moved location instead of at nothing.
+      expect(bookAfter.key).toBe('0_FINISHED/Legacy.m4b');
+      expect(bookAfter.source_path).toBe('Legacy.m4b');
+    });
+
+    it('does not strip the source_path of items relocated earlier in the same batch', async () => {
+      const trx = getTestTransaction();
+      const user = await createTestUser(trx);
+      await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: 'Series',
+        type: 0,
+      });
+      const moved = await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: 'Series/a.m4b',
+        source_path: null,
+      });
+      const stranded = await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: 'Series/b.m4b',
+        source_path: null,
+      });
+      await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: '0_FINISHED',
+        type: 0,
+      });
+      // Only the second child fails to relocate.
+      moveFileMock.mockImplementation(async (params: unknown) => {
+        const { sourceKey } = params as { sourceKey: string };
+        return !sourceKey.endsWith('b.m4b');
+      });
+
+      await service.moveLibraryObject(user as any, {
+        origin: 'Series',
+        destination: '0_FINISHED',
+      });
+
+      const movedAfter = await trx('library_items')
+        .where({ id_library_item: moved.id_library_item })
+        .first();
+      const strandedAfter = await trx('library_items')
+        .where({ id_library_item: stranded.id_library_item })
+        .first();
+      // The one that did relocate keeps the timestamped key naming its new
+      // object (ROOT_FOLDER is not set in CI, so assert the shape, not the
+      // prefix)...
+      expect(movedAfter.key).toBe('0_FINISHED/Series/a.m4b');
+      expect(movedAfter.source_path).toMatch(/_a\.m4b$/);
+      expect(movedAfter.source_path).not.toBe('Series/a.m4b');
+      // ...while the one that did not is pinned to where its bytes still are.
+      expect(strandedAfter.key).toBe('0_FINISHED/Series/b.m4b');
+      expect(strandedAfter.source_path).toBe('Series/b.m4b');
+    });
   });
 
   describe('renameLibraryObject — /rename body { relativePath, newName, uuid }', () => {
diff --git a/src/services/LibraryService.ts b/src/services/LibraryService.ts
index 998a719..e6dbdbd 100644
--- a/src/services/LibraryService.ts
+++ b/src/services/LibraryService.ts
@@ -1253,11 +1253,7 @@ export class LibraryService {
             !fileMoved.source_path &&
             parseInt(fileMoved.type) === parseInt(LibraryItemType.BOOK)
           ) {
-            const suffix =
-              parseInt(fileMoved.type) === parseInt(LibraryItemType.BOOK)
-                ? ''
-                : '/';
-            const sourceKey = `${storagePrefix}/${fileMoved.old_key}${suffix}`;
+            const sourceKey = `${storagePrefix}/${fileMoved.old_key}`;
             const original_filename = `${
               process.env.ROOT_FOLDER
             }/${moment().format('YYYYMMDDHHmmss')}_${
@@ -1268,16 +1264,41 @@ export class LibraryService {
               sourceKey,
               targetKey,
             });
-            if (isMoved) {
-              await this._libraryDB.updateBySourcePath(
+            // Either way the row must end up naming the object that actually
+            // exists. A legacy item (source_path IS NULL) is read back at
+            // `${prefix}/${key}`, so once the key rewrite commits, an object
+            // still sitting at old_key is unreachable — the row would point at
+            // nothing while the bytes are orphaned under the pre-move path.
+            //
+            // Throwing to roll the rewrite back is not an option: the items
+            // relocated earlier in this batch are already at their new keys,
+            // and the rollback would strip the source_path that names them,
+            // orphaning those instead. So on failure we record where the file
+            // really is. The move itself still succeeds — it is a display-path
+            // change — and the item stays playable from its legacy key.
+            if (!isMoved) {
+              this._logger.log(
                 {
-                  user_id: user.id_user,
-                  key: fileMoved.key,
-                  source_path: original_filename,
+                  origin: 'LibraryService.processMovedFiles',
+                  message:
+                    'Storage relocation failed; pinning source_path to the pre-move key',
+                  data: {
+                    id_user: user.id_user,
+                    oldKey: fileMoved.old_key,
+                    newKey: fileMoved.key,
+                  },
                 },
-                trx,
+                'error',
               );
             }
+            await this._libraryDB.updateBySourcePath(
+              {
+                user_id: user.id_user,
+                key: fileMoved.key,
+                source_path: isMoved ? original_filename : fileMoved.old_key,
+              },
+              trx,
+            );
           }
         }
       }),
diff --git a/src/services/S3Service.ts b/src/services/S3Service.ts
index 25ac57d..a860405 100644
--- a/src/services/S3Service.ts
+++ b/src/services/S3Service.ts
@@ -139,12 +139,17 @@ export class S3Service {
       );
       return true;
     } catch (error) {
-      this._logger.log({
-        origin: 'S3: moveFile',
-        message: error.message,
-        data: { sourceKey, targetKey },
-      });
-      return null;
+      // 'error': a failed relocation desynchronizes the DB key from the object
+      // it names, so it has to survive the production LOG_LEVEL of 'warn'.
+      this._logger.log(
+        {
+          origin: 'S3: moveFile',
+          message: error.message,
+          data: { sourceKey, targetKey },
+        },
+        'error',
+      );
+      return false;
     }
   }
 
diff --git a/src/services/StorageService.ts b/src/services/StorageService.ts
index 1c7f876..b232a08 100644
--- a/src/services/StorageService.ts
+++ b/src/services/StorageService.ts
@@ -118,12 +118,17 @@ export class StorageService {
       }
       return moved;
     } catch (error) {
-      this._logger.log({
-        origin: 'Storage: moveFile',
-        message: error.message,
-        data: params,
-      });
-      return null;
+      // See S3Service.moveFile: logged at 'error' so the desync is visible in
+      // production, where LOG_LEVEL is 'warn'.
+      this._logger.log(
+        {
+          origin: 'Storage: moveFile',
+          message: error.message,
+          data: params,
+        },
+        'error',
+      );
+      return false;
     }
   }
 

From 9e6f612452bfb227f7ae58d245c992b09f98c2f0 Mon Sep 17 00:00:00 2001
From: Gianni Carlo <gcarlo89@hotmail.com>
Date: Tue, 15 Sep 2026 11:36:02 -0500
Subject: [PATCH 2/5] fix: address review feedback (round 1)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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.
---
 .../LibraryServicePathMutations.test.ts       | 34 +++++++++++++++++++
 src/services/LibraryService.ts                | 19 +++++++++--
 src/services/S3Service.ts                     | 18 ++++++++--
 src/services/StorageService.ts                | 12 +++++--
 src/utils/index.ts                            | 15 ++++++++
 5 files changed, 91 insertions(+), 7 deletions(-)

diff --git a/src/__tests__/services/LibraryServicePathMutations.test.ts b/src/__tests__/services/LibraryServicePathMutations.test.ts
index 21b427a..4e6fa0a 100644
--- a/src/__tests__/services/LibraryServicePathMutations.test.ts
+++ b/src/__tests__/services/LibraryServicePathMutations.test.ts
@@ -343,6 +343,8 @@ describe('LibraryService — path-mutating flows (move / rename / folder_in_out)
         type: 0,
       });
       moveFileMock.mockImplementation(async () => false);
+      // The copy failed, but the object is still sitting at its old key.
+      fileExistsMock.mockImplementation(async () => true);
 
       await service.moveLibraryObject(user as any, {
         origin: 'Legacy.m4b',
@@ -386,6 +388,7 @@ describe('LibraryService — path-mutating flows (move / rename / folder_in_out)
         const { sourceKey } = params as { sourceKey: string };
         return !sourceKey.endsWith('b.m4b');
       });
+      fileExistsMock.mockImplementation(async () => true);
 
       await service.moveLibraryObject(user as any, {
         origin: 'Series',
@@ -408,6 +411,37 @@ describe('LibraryService — path-mutating flows (move / rename / folder_in_out)
       expect(strandedAfter.key).toBe('0_FINISHED/Series/b.m4b');
       expect(strandedAfter.source_path).toBe('Series/b.m4b');
     });
+
+    it('leaves source_path null when the relocation failed because nothing was at the old key', async () => {
+      const trx = getTestTransaction();
+      const user = await createTestUser(trx);
+      const book = await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: 'Phantom.m4b',
+        source_path: null,
+      });
+      await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: '0_FINISHED',
+        type: 0,
+      });
+      moveFileMock.mockImplementation(async () => false);
+      fileExistsMock.mockImplementation(async () => false);
+
+      await service.moveLibraryObject(user as any, {
+        origin: 'Phantom.m4b',
+        destination: '0_FINISHED',
+      });
+
+      const bookAfter = await trx('library_items')
+        .where({ id_library_item: book.id_library_item })
+        .first();
+      // There are no bytes at either key, so pinning would only record a path
+      // that holds nothing — and freeze the item, since a set source_path
+      // skips the relocation on every later move.
+      expect(bookAfter.key).toBe('0_FINISHED/Phantom.m4b');
+      expect(bookAfter.source_path).toBeNull();
+    });
   });
 
   describe('renameLibraryObject — /rename body { relativePath, newName, uuid }', () => {
diff --git a/src/services/LibraryService.ts b/src/services/LibraryService.ts
index e6dbdbd..2329d81 100644
--- a/src/services/LibraryService.ts
+++ b/src/services/LibraryService.ts
@@ -1277,19 +1277,34 @@ export class LibraryService {
             // really is. The move itself still succeeds — it is a display-path
             // change — and the item stays playable from its legacy key.
             if (!isMoved) {
+              // Two different failures hide behind `false`: the object is
+              // still at old_key and the copy failed, or there was never an
+              // object to copy. Only the first is worth pinning — recording a
+              // path that holds nothing would also freeze the item, since the
+              // guard above skips anything that already has a source_path, so
+              // no later move would retry the relocation.
+              //
+              // fileExists returns null when the probe itself fails, so only a
+              // definitive false counts as "nothing there"; anything else
+              // falls through to pinning, which is right for the common case.
+              const sourceStillThere = await this._storage.fileExists({
+                key: sourceKey,
+              });
               this._logger.log(
                 {
                   origin: 'LibraryService.processMovedFiles',
-                  message:
-                    'Storage relocation failed; pinning source_path to the pre-move key',
+                  message: 'Storage relocation failed',
                   data: {
                     id_user: user.id_user,
                     oldKey: fileMoved.old_key,
                     newKey: fileMoved.key,
+                    sourceStillThere,
+                    pinned: sourceStillThere !== false,
                   },
                 },
                 'error',
               );
+              if (sourceStillThere === false) continue;
             }
             await this._libraryDB.updateBySourcePath(
               {
diff --git a/src/services/S3Service.ts b/src/services/S3Service.ts
index a860405..daebf9a 100644
--- a/src/services/S3Service.ts
+++ b/src/services/S3Service.ts
@@ -17,6 +17,7 @@ import { S3ClientHeaders, StorageAction, StorageItem } from '../types/user';
 import moment from 'moment';
 import { logger } from './LoggerService';
 import { Readable } from 'stream';
+import { stripStoragePrefix } from '../utils';
 
 export class S3Service {
   private readonly _logger = logger;
@@ -121,6 +122,11 @@ export class S3Service {
   }
 
   async moveFile(sourceKey: string, targetKey: string): Promise<boolean> {
+    // Copy-then-delete, so a failure has two very different shapes: the copy
+    // 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;
     try {
       await this.clientObject.send(
         new CopyObjectCommand({
@@ -131,6 +137,7 @@ export class S3Service {
           )}`,
         }),
       );
+      copied = true;
       await this.clientObject.send(
         new DeleteObjectCommand({
           Bucket: process.env.S3_BUCKET,
@@ -141,11 +148,18 @@ export class S3Service {
     } catch (error) {
       // 'error': a failed relocation desynchronizes the DB key from the object
       // it names, so it has to survive the production LOG_LEVEL of 'warn'.
+      // Keys are prefix-stripped: for legacy accounts that prefix is the user's
+      // email, and this path serves exactly those accounts.
       this._logger.log(
         {
-          origin: 'S3: moveFile',
+          origin: 'S3Service.moveFile',
           message: error.message,
-          data: { sourceKey, targetKey },
+          data: {
+            sourceKey: stripStoragePrefix(sourceKey),
+            targetKey: stripStoragePrefix(targetKey),
+            copied,
+            errorName: error.name,
+          },
         },
         'error',
       );
diff --git a/src/services/StorageService.ts b/src/services/StorageService.ts
index b232a08..8dbe471 100644
--- a/src/services/StorageService.ts
+++ b/src/services/StorageService.ts
@@ -7,6 +7,7 @@ import {
 import { logger } from './LoggerService';
 import { S3Service } from './S3Service';
 import { Readable } from 'stream';
+import { stripStoragePrefix } from '../utils';
 
 export class StorageService {
   private readonly _logger = logger;
@@ -119,12 +120,17 @@ export class StorageService {
       return moved;
     } catch (error) {
       // See S3Service.moveFile: logged at 'error' so the desync is visible in
-      // production, where LOG_LEVEL is 'warn'.
+      // production, where LOG_LEVEL is 'warn'. `params` is not logged whole —
+      // its keys carry the per-user storage prefix, which is the account's
+      // email for the legacy accounts this path serves.
       this._logger.log(
         {
-          origin: 'Storage: moveFile',
+          origin: 'StorageService.moveFile',
           message: error.message,
-          data: params,
+          data: {
+            sourceKey: stripStoragePrefix(params.sourceKey),
+            targetKey: stripStoragePrefix(params.targetKey),
+          },
         },
         'error',
       );
diff --git a/src/utils/index.ts b/src/utils/index.ts
index bcba6b2..84d8267 100644
--- a/src/utils/index.ts
+++ b/src/utils/index.ts
@@ -28,6 +28,21 @@ export function isValidUUID(testUuid?: string): boolean {
   return UUID_REGEX.test(testUuid);
 }
 
+/**
+ * Drops the per-user storage prefix from an S3 key so the remainder is safe to
+ * log. The prefix is always a single segment — `users.external_id`, or, for
+ * legacy accounts, the account's email address (see StoragePrefixService) —
+ * and LoggerService redacts only password/token/secret/authorization, so a
+ * whole key logged verbatim would publish that address.
+ * @param storageKey - A prefixed key, e.g. `someone@example.com/root/1_a.mp3`
+ * @returns The key without its first segment, e.g. `root/1_a.mp3`
+ */
+export const stripStoragePrefix = (storageKey?: string): string => {
+  if (!storageKey) return '';
+  const separator = storageKey.indexOf('/');
+  return separator === -1 ? '' : storageKey.slice(separator + 1);
+};
+
 export const splitArrayGroups = (array: unknown[], chunkSize: number) => {
   const chunks = [];
   for (let i = 0; i < array.length; i += chunkSize) {

From 90efd2e9c15aa3f46da07fdfd0bd64ad598291c6 Mon Sep 17 00:00:00 2001
From: Gianni Carlo <gcarlo89@hotmail.com>
Date: Tue, 15 Sep 2026 11:43:41 -0500
Subject: [PATCH 3/5] fix: address review feedback (round 2)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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.
---
 .../LibraryServicePathMutations.test.ts       | 67 +++++++++++++++++++
 .../utils/stripStoragePrefix.test.ts          | 37 ++++++++++
 src/services/LibraryService.ts                | 47 +++++++++----
 src/services/S3Service.ts                     |  8 ++-
 src/services/StorageService.ts                |  5 +-
 5 files changed, 148 insertions(+), 16 deletions(-)
 create mode 100644 src/__tests__/utils/stripStoragePrefix.test.ts

diff --git a/src/__tests__/services/LibraryServicePathMutations.test.ts b/src/__tests__/services/LibraryServicePathMutations.test.ts
index 4e6fa0a..c156559 100644
--- a/src/__tests__/services/LibraryServicePathMutations.test.ts
+++ b/src/__tests__/services/LibraryServicePathMutations.test.ts
@@ -442,6 +442,73 @@ describe('LibraryService — path-mutating flows (move / rename / folder_in_out)
       expect(bookAfter.key).toBe('0_FINISHED/Phantom.m4b');
       expect(bookAfter.source_path).toBeNull();
     });
+
+    it('pins to the new key when the delete landed but its response was lost', async () => {
+      const trx = getTestTransaction();
+      const user = await createTestUser(trx);
+      const book = await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: 'Lost.m4b',
+        source_path: null,
+      });
+      await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: '0_FINISHED',
+        type: 0,
+      });
+      // moveFile is copy-then-delete: the copy landed and the delete actually
+      // ran on S3, but the SDK call threw, so the move reports failure while
+      // the bytes are already at the target and gone from the source.
+      moveFileMock.mockImplementation(async () => false);
+      fileExistsMock.mockImplementation(async (params: unknown) => {
+        const { key } = params as { key: string };
+        return key !== 'test-prefix/Lost.m4b';
+      });
+
+      await service.moveLibraryObject(user as any, {
+        origin: 'Lost.m4b',
+        destination: '0_FINISHED',
+      });
+
+      const bookAfter = await trx('library_items')
+        .where({ id_library_item: book.id_library_item })
+        .first();
+      // The move really did happen, so pin to the target, not the dead source.
+      expect(bookAfter.key).toBe('0_FINISHED/Lost.m4b');
+      expect(bookAfter.source_path).not.toBeNull();
+      expect(bookAfter.source_path).not.toBe('Lost.m4b');
+      expect(bookAfter.source_path).toMatch(/_Lost\.m4b$/);
+    });
+
+    it('still pins to the old key when the existence probe itself fails', async () => {
+      const trx = getTestTransaction();
+      const user = await createTestUser(trx);
+      const book = await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: 'Probe.m4b',
+        source_path: null,
+      });
+      await createTestLibraryItem(trx, {
+        user_id: user.id_user,
+        key: '0_FINISHED',
+        type: 0,
+      });
+      moveFileMock.mockImplementation(async () => false);
+      // null = could not determine. Must not be read as "absent", or the row
+      // would be left naming nothing — the bug this whole path guards against.
+      fileExistsMock.mockImplementation(async () => null);
+
+      await service.moveLibraryObject(user as any, {
+        origin: 'Probe.m4b',
+        destination: '0_FINISHED',
+      });
+
+      const bookAfter = await trx('library_items')
+        .where({ id_library_item: book.id_library_item })
+        .first();
+      expect(bookAfter.key).toBe('0_FINISHED/Probe.m4b');
+      expect(bookAfter.source_path).toBe('Probe.m4b');
+    });
   });
 
   describe('renameLibraryObject — /rename body { relativePath, newName, uuid }', () => {
diff --git a/src/__tests__/utils/stripStoragePrefix.test.ts b/src/__tests__/utils/stripStoragePrefix.test.ts
new file mode 100644
index 0000000..c6bf2c5
--- /dev/null
+++ b/src/__tests__/utils/stripStoragePrefix.test.ts
@@ -0,0 +1,37 @@
+import { describe, it, expect } from '@jest/globals';
+import { stripStoragePrefix } from '../../utils';
+
+/**
+ * stripStoragePrefix is what keeps a legacy account's email address out of
+ * CloudWatch: the per-user storage prefix is `users.external_id`, or that
+ * address for accounts predating it (see StoragePrefixService). These cases
+ * pin the edges so a later simplification cannot quietly reintroduce the leak.
+ */
+describe('stripStoragePrefix', () => {
+  it('drops the prefix segment and keeps the rest of the key', () => {
+    expect(stripStoragePrefix('someone@example.com/root/1_a.mp3')).toBe(
+      'root/1_a.mp3',
+    );
+  });
+
+  it('keeps every segment after the first', () => {
+    expect(
+      stripStoragePrefix('someone@example.com/TV/Bluey/S01/[E01] Bike.mp3'),
+    ).toBe('TV/Bluey/S01/[E01] Bike.mp3');
+  });
+
+  it('returns empty for a bare prefix, rather than echoing the address', () => {
+    expect(stripStoragePrefix('someone@example.com')).toBe('');
+  });
+
+  it('returns empty for missing or empty input', () => {
+    expect(stripStoragePrefix(undefined)).toBe('');
+    expect(stripStoragePrefix('')).toBe('');
+  });
+
+  it('handles an external_id prefix the same way', () => {
+    expect(
+      stripStoragePrefix('001172.22b2d822a90b45bf8c4d250c3dda4d6a.1714/root/x.m4b'),
+    ).toBe('root/x.m4b');
+  });
+});
diff --git a/src/services/LibraryService.ts b/src/services/LibraryService.ts
index 2329d81..4a66932 100644
--- a/src/services/LibraryService.ts
+++ b/src/services/LibraryService.ts
@@ -1276,20 +1276,40 @@ export class LibraryService {
             // orphaning those instead. So on failure we record where the file
             // really is. The move itself still succeeds — it is a display-path
             // change — and the item stays playable from its legacy key.
+            // Where the object is, as best we can establish it.
+            let pinnedSourcePath = original_filename;
+
             if (!isMoved) {
-              // Two different failures hide behind `false`: the object is
-              // still at old_key and the copy failed, or there was never an
-              // object to copy. Only the first is worth pinning — recording a
-              // path that holds nothing would also freeze the item, since the
-              // guard above skips anything that already has a source_path, so
-              // no later move would retry the relocation.
-              //
-              // fileExists returns null when the probe itself fails, so only a
-              // definitive false counts as "nothing there"; anything else
-              // falls through to pinning, which is right for the common case.
+              // A failed move does not locate the bytes on its own. moveFile
+              // is copy-then-delete, so the failure may be a copy that never
+              // landed (bytes at old_key) or a delete that landed on S3 but
+              // 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({
                 key: sourceKey,
               });
+              const targetLanded =
+                sourceStillThere === false
+                  ? await this._storage.fileExists({ key: targetKey })
+                  : null;
+
+              // Anything short of a definitive "source is gone" means the
+              // object is, or is presumed, still at the old key — the common
+              // case, and the safe default when the probe itself failed
+              // (fileExists returns null then).
+              if (sourceStillThere !== false) {
+                pinnedSourcePath = fileMoved.old_key;
+              }
+
+              // Nothing found at either key. Pinning would record a path that
+              // holds nothing and freeze the row: the guard above skips items
+              // that already have a source_path, so no later move would retry
+              // the relocation. Leave it null instead — still broken, but
+              // still detectable and still retryable.
+              const foundNothing =
+                sourceStillThere === false && targetLanded !== true;
+
               this._logger.log(
                 {
                   origin: 'LibraryService.processMovedFiles',
@@ -1299,18 +1319,19 @@ export class LibraryService {
                     oldKey: fileMoved.old_key,
                     newKey: fileMoved.key,
                     sourceStillThere,
-                    pinned: sourceStillThere !== false,
+                    targetLanded,
+                    pinnedSourcePath: foundNothing ? null : pinnedSourcePath,
                   },
                 },
                 'error',
               );
-              if (sourceStillThere === false) continue;
+              if (foundNothing) continue;
             }
             await this._libraryDB.updateBySourcePath(
               {
                 user_id: user.id_user,
                 key: fileMoved.key,
-                source_path: isMoved ? original_filename : fileMoved.old_key,
+                source_path: pinnedSourcePath,
               },
               trx,
             );
diff --git a/src/services/S3Service.ts b/src/services/S3Service.ts
index daebf9a..214d9fa 100644
--- a/src/services/S3Service.ts
+++ b/src/services/S3Service.ts
@@ -24,7 +24,13 @@ export class S3Service {
   private client = new S3({ region: process.env.S3_REGION });
   private clientObject = new S3Client({ region: process.env.S3_REGION });
 
-  async fileExists(key: string): Promise<boolean> {
+  /**
+   * Tri-state on purpose: true/false are definitive, null means the probe
+   * itself failed and the caller must not read that as "absent". Note a 403
+   * also yields false — on a bucket without s3:ListBucket that is how a
+   * permission problem surfaces, not proof the key is missing.
+   */
+  async fileExists(key: string): Promise<boolean | null> {
     try {
       const data = await this.client.headObject({
         Bucket: process.env.S3_BUCKET,
diff --git a/src/services/StorageService.ts b/src/services/StorageService.ts
index 8dbe471..5da2543 100644
--- a/src/services/StorageService.ts
+++ b/src/services/StorageService.ts
@@ -14,14 +14,15 @@ export class StorageService {
 
   constructor(private _s3Service: S3Service = new S3Service()) {}
 
+  /** Tri-state; see S3Service.fileExists. null means "could not determine". */
   async fileExists(params: {
     key: string;
     origin?: StorageOrigin;
-  }): Promise<boolean> {
+  }): Promise<boolean | null> {
     try {
       const { key, origin } = params;
       const storageOrigin = origin || StorageOrigin.S3;
-      let exist = false;
+      let exist: boolean | null = false;
       switch (storageOrigin) {
         case StorageOrigin.S3:
           exist = await this._s3Service.fileExists(key);

From c15bea998b39daa7a002ba0e114ba1f13e3080b7 Mon Sep 17 00:00:00 2001
From: Gianni Carlo <gcarlo89@hotmail.com>
Date: Tue, 15 Sep 2026 11:50:12 -0500
Subject: [PATCH 4/5] fix: address review feedback (round 3)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../services/S3ServiceFileExists.test.ts      | 70 +++++++++++++++++++
 src/services/S3Service.ts                     | 27 +++++--
 2 files changed, 91 insertions(+), 6 deletions(-)
 create mode 100644 src/__tests__/services/S3ServiceFileExists.test.ts

diff --git a/src/__tests__/services/S3ServiceFileExists.test.ts b/src/__tests__/services/S3ServiceFileExists.test.ts
new file mode 100644
index 0000000..97364e7
--- /dev/null
+++ b/src/__tests__/services/S3ServiceFileExists.test.ts
@@ -0,0 +1,70 @@
+import { describe, it, expect, beforeEach, jest } from '@jest/globals';
+import { S3Service } from '../../services/S3Service';
+import { mockLoggerService } from '../setup';
+
+/**
+ * fileExists is tri-state, and LibraryService.processMovedFiles depends on the
+ * distinction: only a definitive `false` from BOTH the source and target probe
+ * lets it conclude a moved item's bytes are nowhere and leave source_path null.
+ * A probe that merely could not determine the answer must come back as null, or
+ * that path records "nothing exists" on an object it was simply unable to read.
+ */
+describe('S3Service.fileExists — tri-state', () => {
+  let service: S3Service;
+  let headObjectMock: jest.Mock;
+
+  beforeEach(() => {
+    service = new S3Service();
+    headObjectMock = jest.fn();
+    (service as any).client = { headObject: headObjectMock };
+    (service as any)._logger = mockLoggerService;
+    mockLoggerService.log.mockClear();
+  });
+
+  it('returns true when the object is there', async () => {
+    headObjectMock.mockImplementation(async () => ({
+      $metadata: { httpStatusCode: 200 },
+    }));
+    await expect(service.fileExists('prefix/root/a.m4b')).resolves.toBe(true);
+  });
+
+  it('returns false for a 404 — definitively absent', async () => {
+    headObjectMock.mockImplementation(async () => {
+      throw Object.assign(new Error('Not Found'), {
+        $metadata: { httpStatusCode: 404 },
+      });
+    });
+    await expect(service.fileExists('prefix/root/a.m4b')).resolves.toBe(false);
+  });
+
+  it('returns null for a 403 — denied is not the same as absent', async () => {
+    headObjectMock.mockImplementation(async () => {
+      throw Object.assign(new Error('Forbidden'), {
+        $metadata: { httpStatusCode: 403 },
+      });
+    });
+    await expect(service.fileExists('prefix/root/a.m4b')).resolves.toBeNull();
+  });
+
+  it('returns null for any other failure', async () => {
+    headObjectMock.mockImplementation(async () => {
+      throw Object.assign(new Error('boom'), {
+        $metadata: { httpStatusCode: 500 },
+      });
+    });
+    await expect(service.fileExists('prefix/root/a.m4b')).resolves.toBeNull();
+  });
+
+  it('does not log the storage prefix, which can be the account email', async () => {
+    headObjectMock.mockImplementation(async () => {
+      throw Object.assign(new Error('Forbidden'), {
+        $metadata: { httpStatusCode: 403 },
+      });
+    });
+    await service.fileExists('someone@example.com/root/a.m4b');
+
+    const logged = JSON.stringify(mockLoggerService.log.mock.calls);
+    expect(logged).not.toContain('someone@example.com');
+    expect(logged).toContain('root/a.m4b');
+  });
+});
diff --git a/src/services/S3Service.ts b/src/services/S3Service.ts
index 214d9fa..5716779 100644
--- a/src/services/S3Service.ts
+++ b/src/services/S3Service.ts
@@ -26,9 +26,12 @@ export class S3Service {
 
   /**
    * Tri-state on purpose: true/false are definitive, null means the probe
-   * itself failed and the caller must not read that as "absent". Note a 403
-   * also yields false — on a bucket without s3:ListBucket that is how a
-   * permission problem surfaces, not proof the key is missing.
+   * could not determine it and the caller must not read that as "absent".
+   *
+   * A 403 is indeterminate, not absent. S3 masks a missing key as 403 only
+   * when the caller lacks s3:ListBucket, and this role holds it (see
+   * getDirectoryContent / calculateFolderSize, which call ListObjectsV2), so
+   * a 403 here means a permission or KMS problem rather than a missing key.
    */
   async fileExists(key: string): Promise<boolean | null> {
     try {
@@ -42,12 +45,24 @@ export class S3Service {
       if (error.$metadata?.httpStatusCode === 404) {
         return false;
       } else if (error.$metadata?.httpStatusCode === 403) {
-        return false;
+        // Indeterminate, not absent — see the tri-state note above. Returning
+        // false here would let a permission failure read as "the object is
+        // nowhere", which is how a caller ends up recording that nothing
+        // exists when in fact it could not look.
+        this._logger.log(
+          {
+            origin: 'S3Service.fileExists',
+            message: 'Existence probe denied (403); treating as indeterminate',
+            data: { key: stripStoragePrefix(key) },
+          },
+          'warn',
+        );
+        return null;
       } else {
         this._logger.log({
-          origin: 'S3: fileExists',
+          origin: 'S3Service.fileExists',
           message: error.message,
-          data: { key },
+          data: { key: stripStoragePrefix(key) },
         });
         return null;
       }

From fc2e185c00a8790d882af01b1b3ad19f932111c5 Mon Sep 17 00:00:00 2001
From: Gianni Carlo <gcarlo89@hotmail.com>
Date: Tue, 15 Sep 2026 11:57:06 -0500
Subject: [PATCH 5/5] fix: address review feedback (round 4)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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.
---
 src/services/LibraryService.ts |  6 ++++++
 src/services/S3Service.ts      | 16 +++++++++++-----
 src/services/StorageService.ts | 16 +++++++++++-----
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/src/services/LibraryService.ts b/src/services/LibraryService.ts
index 4a66932..e36cca1 100644
--- a/src/services/LibraryService.ts
+++ b/src/services/LibraryService.ts
@@ -1320,6 +1320,12 @@ export class LibraryService {
                     newKey: fileMoved.key,
                     sourceStillThere,
                     targetLanded,
+                    // The decision itself, not just its inputs. A remediation
+                    // sweep can then separate a confirmed phantom (both
+                    // probes 404) from one where the target probe only came
+                    // back indeterminate.
+                    foundNothing,
+                    targetIndeterminate: targetLanded === null,
                     pinnedSourcePath: foundNothing ? null : pinnedSourcePath,
                   },
                 },
diff --git a/src/services/S3Service.ts b/src/services/S3Service.ts
index 5716779..19d1902 100644
--- a/src/services/S3Service.ts
+++ b/src/services/S3Service.ts
@@ -59,11 +59,17 @@ export class S3Service {
         );
         return null;
       } else {
-        this._logger.log({
-          origin: 'S3Service.fileExists',
-          message: error.message,
-          data: { key: stripStoragePrefix(key) },
-        });
+        // Same level as the 403 branch: this is the wider indeterminate class
+        // (5xx, timeouts, SDK failures) and it drives the same caller
+        // decision, so it has to clear the production LOG_LEVEL of 'warn' too.
+        this._logger.log(
+          {
+            origin: 'S3Service.fileExists',
+            message: error.message,
+            data: { key: stripStoragePrefix(key), errorName: error.name },
+          },
+          'warn',
+        );
         return null;
       }
     }
diff --git a/src/services/StorageService.ts b/src/services/StorageService.ts
index 5da2543..bf61c0a 100644
--- a/src/services/StorageService.ts
+++ b/src/services/StorageService.ts
@@ -32,11 +32,17 @@ export class StorageService {
       }
       return exist;
     } catch (error) {
-      this._logger.log({
-        origin: 'Storage: fileExists',
-        message: error.message,
-        data: params,
-      });
+      // Prefix-stripped and at 'warn' for the same reasons as the moveFile
+      // catches: params.key carries the per-user prefix, which is the account
+      // email for legacy accounts, and this null drives the caller's pin.
+      this._logger.log(
+        {
+          origin: 'StorageService.fileExists',
+          message: error.message,
+          data: { key: stripStoragePrefix(params.key) },
+        },
+        'warn',
+      );
       return null;
     }
   }