From 0df57b62b31bf1b86cdcee6b77d04be027fe58f0 Mon Sep 17 00:00:00 2001 From: Richard R Date: Sat, 6 Jun 2026 20:03:34 -0600 Subject: [PATCH] refactor(data): improve error handling and rollback for document and TTS segment operations Enhance robustness of document deletion and TTS segment transfer by improving error logging, partial rollback, and degraded state reporting. Add best-effort cleanup and recovery mechanisms to prevent orphaned data and ensure diagnostic information is captured for unexpected failures. Update user data cleanup to log and swallow restoration errors without masking primary failures. Refine claim logic to handle unmapped TTS audio keys safely. --- src/app/api/documents/route.ts | 4 -- src/components/auth/ClaimDataModal.tsx | 4 +- src/lib/server/documents/blobstore.ts | 24 ++++++++++- src/lib/server/tts/segments-blobstore.ts | 51 +++++++++++++++--------- src/lib/server/user/claim-data.ts | 39 ++++++++++++++---- src/lib/server/user/data-cleanup.ts | 19 ++++++++- 6 files changed, 104 insertions(+), 37 deletions(-) diff --git a/src/app/api/documents/route.ts b/src/app/api/documents/route.ts index a414d4d..c7d21d8 100644 --- a/src/app/api/documents/route.ts +++ b/src/app/api/documents/route.ts @@ -111,10 +111,6 @@ export async function DELETE(req: NextRequest) { const targetUserIds = [storageUserId]; - if (targetUserIds.length === 0) { - return NextResponse.json({ success: true, deleted: 0 }); - } - let targetIds: string[] = []; if (idsParam) { targetIds = idsParam diff --git a/src/components/auth/ClaimDataModal.tsx b/src/components/auth/ClaimDataModal.tsx index 7811398..f1338da 100644 --- a/src/components/auth/ClaimDataModal.tsx +++ b/src/components/auth/ClaimDataModal.tsx @@ -52,8 +52,8 @@ export default function ClaimDataModal({ toast.success( `Successfully claimed ${claimed.documents} documents, ` + `${claimed.audiobooks} audiobooks, ` - + `${claimed.preferences} preference set(s), and ` - + `${claimed.progress} reading progress record(s) and ` + + `${claimed.preferences} preference set(s), ` + + `${claimed.progress} reading progress record(s), and ` + `${claimed.documentSettings} document setting(s)!`, ); onClaimed(); diff --git a/src/lib/server/documents/blobstore.ts b/src/lib/server/documents/blobstore.ts index 5b4a85a..e3a8673 100644 --- a/src/lib/server/documents/blobstore.ts +++ b/src/lib/server/documents/blobstore.ts @@ -10,6 +10,8 @@ import { import { getSignedUrl } from '@aws-sdk/s3-request-presigner'; import { PDF_PARSER_VERSION, encodeParserVersion } from '@openreader/compute-core/api-contracts'; import { getS3Client, getS3Config, getS3ProxyClient } from '@/lib/server/storage/s3'; +import { serverLogger } from '@/lib/server/logger'; +import { logDegraded } from '@/lib/server/errors/logging'; const DOCUMENT_ID_REGEX = /^[a-f0-9]{64}$/i; const SAFE_NAMESPACE_REGEX = /^[a-zA-Z0-9._-]{1,128}$/; @@ -513,7 +515,20 @@ export async function deleteDocumentBlob(id: string, namespace: string | null): // Delete the source after the initial derived-artifact cleanup, then sweep // parsed output once more to catch a worker that finished during deletion. await client.send(new DeleteObjectCommand({ Bucket: cfg.bucket, Key: key })); - await deleteDocumentPrefix(parsedPrefix); + // The source blob is already gone at this point. Treat the final sweep as a + // best-effort cleanup: if it throws, rethrowing would make callers roll back + // the document row even though the source is deleted, so log and swallow. + try { + await deleteDocumentPrefix(parsedPrefix); + } catch (error) { + logDegraded(serverLogger, { + event: 'documents.blob_delete.final_parsed_sweep.failed', + msg: 'Failed final parsed-output sweep after document deletion', + step: 'delete_document_parsed_prefix_final', + context: { parsedPrefix }, + error, + }); + } } export async function deleteTempDocumentUpload(token: string, userId: string, namespace: string | null): Promise { @@ -569,7 +584,12 @@ export async function deleteDocumentPrefix(prefix: string): Promise { ); const errors = deleteRes.Errors ?? []; if (errors.length > 0) { - throw new Error(`Failed deleting ${errors.length} document storage objects`); + const details = errors + .map((e) => `${e.Key ?? '?'} (${e.Code ?? 'Unknown'}: ${e.Message ?? 'no message'})`) + .join('; '); + throw new Error( + `Failed deleting ${errors.length} document storage object(s) under prefix "${cleanedPrefix}": ${details}`, + ); } deleted += keys.length; } diff --git a/src/lib/server/tts/segments-blobstore.ts b/src/lib/server/tts/segments-blobstore.ts index a6693bd..529f69f 100644 --- a/src/lib/server/tts/segments-blobstore.ts +++ b/src/lib/server/tts/segments-blobstore.ts @@ -268,28 +268,41 @@ export async function copyTtsSegmentPrefix(sourcePrefix: string, destinationPref let copied = 0; let continuationToken: string | undefined; - do { - const listRes = await client.send(new ListObjectsV2Command({ - Bucket: cfg.bucket, - Prefix: source, - ContinuationToken: continuationToken, - })); - const keys = (listRes.Contents ?? []) - .map((item) => item.Key) - .filter((value): value is string => typeof value === 'string' && value.startsWith(source)); - - for (const key of keys) { - await client.send(new CopyObjectCommand({ + // Track destination keys we have written so a mid-copy failure can be rolled + // back, leaving no orphaned objects behind at the destination prefix. + const copiedKeys: string[] = []; + try { + do { + const listRes = await client.send(new ListObjectsV2Command({ Bucket: cfg.bucket, - Key: `${destination}${key.slice(source.length)}`, - CopySource: `${cfg.bucket}/${key}`, - ServerSideEncryption: 'AES256', + Prefix: source, + ContinuationToken: continuationToken, })); - copied += 1; - } + const keys = (listRes.Contents ?? []) + .map((item) => item.Key) + .filter((value): value is string => typeof value === 'string' && value.startsWith(source)); - continuationToken = listRes.IsTruncated ? listRes.NextContinuationToken : undefined; - } while (continuationToken); + for (const key of keys) { + const destinationKey = `${destination}${key.slice(source.length)}`; + await client.send(new CopyObjectCommand({ + Bucket: cfg.bucket, + Key: destinationKey, + CopySource: `${cfg.bucket}/${key}`, + ServerSideEncryption: 'AES256', + })); + copiedKeys.push(destinationKey); + copied += 1; + } + + continuationToken = listRes.IsTruncated ? listRes.NextContinuationToken : undefined; + } while (continuationToken); + } catch (error) { + // Best-effort rollback of the partial copy; surface the original error. + if (copiedKeys.length > 0) { + await deleteTtsSegmentAudioObjects(copiedKeys).catch(() => {}); + } + throw error; + } return copied; } diff --git a/src/lib/server/user/claim-data.ts b/src/lib/server/user/claim-data.ts index 389429c..64882d6 100644 --- a/src/lib/server/user/claim-data.ts +++ b/src/lib/server/user/claim-data.ts @@ -301,16 +301,39 @@ async function transferDocumentTtsSegments(input: { const encodedFrom = encodeURIComponent(input.fromUserId); const encodedTo = encodeURIComponent(input.toUserId); + const sourceAudioKeyPrefix = `/users/${encodedFrom}/docs/${input.documentId}/`; + const destAudioKeyPrefix = `/users/${encodedTo}/docs/${input.documentId}/`; if (variants.length > 0) { await input.database.insert(ttsSegmentVariants) - .values(variants.map((variant: typeof ttsSegmentVariants.$inferSelect) => ({ - ...variant, - userId: input.toUserId, - audioKey: variant.audioKey?.replace( - `/users/${encodedFrom}/docs/${input.documentId}/`, - `/users/${encodedTo}/docs/${input.documentId}/`, - ) ?? null, - }))) + .values(variants.map((variant: typeof ttsSegmentVariants.$inferSelect) => { + const audioKey = variant.audioKey ?? null; + if (!audioKey || audioKey.includes(sourceAudioKeyPrefix)) { + return { + ...variant, + userId: input.toUserId, + audioKey: audioKey?.replace(sourceAudioKeyPrefix, destAudioKeyPrefix) ?? null, + }; + } + // The key did not contain the expected source path, so it cannot be + // safely remapped. Leaving it would point the new owner at the source + // user's (soon-deleted) audio, so null it out and log for investigation. + logDegraded(serverLogger, { + event: 'user.claim.tts_variant_audio_key.unmapped', + msg: 'TTS segment variant audioKey did not match expected source path during claim', + step: 'remap_tts_variant_audio_key', + context: { + originalAudioKey: audioKey, + fromUserIdHash: hashForLog(input.fromUserId), + toUserIdHash: hashForLog(input.toUserId), + documentId: input.documentId, + }, + }); + return { + ...variant, + userId: input.toUserId, + audioKey: null, + }; + })) .onConflictDoNothing(); } diff --git a/src/lib/server/user/data-cleanup.ts b/src/lib/server/user/data-cleanup.ts index b90bffe..52d7154 100644 --- a/src/lib/server/user/data-cleanup.ts +++ b/src/lib/server/user/data-cleanup.ts @@ -61,6 +61,21 @@ export async function deleteUserStorageData( await database.insert(documents).values(docsToRestore).onConflictDoNothing(); removedDocs.length = 0; }; + // Restore before throwing the diagnostic AggregateError, but never let a + // restore failure replace the original cleanup failures we want to surface. + const tryRestoreRemovedDocs = async () => { + try { + await restoreRemovedDocs(); + } catch (error) { + logDegraded(serverLogger, { + event: 'user.data_cleanup.restore_removed_docs.failed', + msg: 'Failed to restore document rows after cleanup failure', + step: 'restore_removed_docs', + context: { userIdHash: hashForLog(userId) }, + error, + }); + } + }; const removeOwnershipAndCheckLastOwner = async (doc: DocumentRow) => { const run = async (tx: typeof database) => { // Lock every ownership row for this document so concurrent account @@ -248,7 +263,7 @@ export async function deleteUserStorageData( } if (failures.length > 0) { - await restoreRemovedDocs(); + await tryRestoreRemovedDocs(); throw new AggregateError(failures, `User storage cleanup failed in ${failures.length} operation(s)`); } @@ -289,7 +304,7 @@ export async function deleteUserStorageData( } if (failures.length > 0) { - await restoreRemovedDocs(); + await tryRestoreRemovedDocs(); throw new AggregateError(failures, `User database cleanup failed in ${failures.length} operation(s)`); } }