Improve batch worker error log serialization and test assertions

.eslintrc.cjs

@@ -2,5 +2,18 @@ module.exports = {
     plugins: ['ghost'],
     extends: [
         'plugin:ghost/ts'
-    ]
+    ],
+    rules: {
+        'no-restricted-syntax': [
+            'error',
+            {
+                selector: 'CallExpression[callee.object.name=\'logger\'] > ObjectExpression > Property[key.name=\'error\']',
+                message: 'Use `err` instead of `error` — Pino only serializes errors on the `err` key.'
+            },
+            {
+                selector: 'CallExpression[callee.object.property.name=\'log\'] > ObjectExpression > Property[key.name=\'error\']',
+                message: 'Use `err` instead of `error` — Pino only serializes errors on the `err` key.'
+            }
+        ]
+    }
 };

src/handlers/page-hit-handlers.ts

@@ -10,14 +10,10 @@ export const handlePageHitRequestStrategyBatch = async (request: PageHitRequestT
     try {
         await publishPageHitRaw(request, payload);
         reply.status(202).send({message: 'Page hit event received'});
-    } catch (error) {
+    } catch (err) {
         request.log.error({
             event: 'PageHitPublishFailed',
-            error: error instanceof Error ? {
-                message: error.message,
-                stack: error.stack,
-                name: error.name
-            } : error,
+            err,
             payload,
             httpRequest: {
                 requestMethod: request.method,
@@ -80,11 +76,7 @@ export const handlePageHitRequestStrategyInline = async (request: PageHitRequest
             const unwrappedError = 'error' in error ? error.error : error;
             replyInstance.log.error({
                 event: 'ProxyError',
-                error: {
-                    message: unwrappedError.message,
-                    stack: unwrappedError.stack,
-                    name: unwrappedError.name
-                },
+                err: unwrappedError,
                 httpRequest: {
                     requestMethod: replyInstance.request.method,
                     requestUrl: replyInstance.request.url,
@@ -114,14 +106,10 @@ export const pageHitRequestHandler = async (request: FastifyRequest<{
         } else {
             await handlePageHitRequestStrategyInline(request, reply);
         }
-    } catch (error) {
+    } catch (err) {
         reply.log.error({
             event: 'RequestProcessingError',
-            error: error instanceof Error ? {
-                message: error.message,
-                stack: error.stack,
-                name: error.name
-            } : error,
+            err,
             httpRequest: {
                 requestMethod: request.method,
                 requestUrl: request.url,

src/plugins/hmac-validation.ts

@@ -33,7 +33,7 @@ async function hmacValidationPlugin(fastify: FastifyInstance) {
                     method: request.method,
                     ip: request.ip,
                     userAgent: request.headers['user-agent'],
-                    error: validationResult.error,
+                    reason: validationResult.error,
                     type: 'security_validation_error',
                     logOnlyMode
                 });
@@ -64,14 +64,10 @@ async function hmacValidationPlugin(fastify: FastifyInstance) {
                 method: request.method,
                 ip: request.ip
             });
-        } catch (error) {
+        } catch (err) {
             request.log.error({
                 event: 'HmacValidationError',
-                error: error instanceof Error ? {
-                    message: error.message,
-                    stack: error.stack,
-                    name: error.name
-                } : error,
+                err,
                 url: request.url,
                 method: request.method,
                 ip: request.ip,

src/services/batch-worker/BatchWorker.ts

@@ -100,12 +100,12 @@ class BatchWorker {
             if (this.batch.length >= this.batchSize) {
                 await this.flushBatch();
             }
-        } catch (error) {
+        } catch (err) {
             logger.error({
                 event: 'WorkerMessageProcessingFailed',
                 messageId: message.id,
                 messageData: this.getMessageData(message),
-                error
+                err
             });
             message.nack();
         }
@@ -116,12 +116,12 @@ class BatchWorker {
             const messageData = message.data.toString();
             const parsedMessageData = JSON.parse(messageData);
             return Value.Parse(PageHitRawSchema, parsedMessageData);
-        } catch (error) {
+        } catch (err) {
             logger.error({
                 event: 'WorkerMessageParsingFailed',
                 messageId: message.id,
                 messageData: this.getMessageData(message),
-                error
+                err
             });
             // Ack the message. If we failed to parse it, we won't succeed next time.
             message.ack();
@@ -155,20 +155,20 @@ class BatchWorker {
                 batchSize: batchToFlush.length,
                 messageIds: batchToFlush.map(item => item.message.id)
             });
-        } catch (error) {
+        } catch (err) {
             logger.error({
                 event: 'WorkerBatchFlushFailed',
                 batchSize: batchToFlush.length,
                 messageIds: batchToFlush.map(item => item.message.id),
-                error
+                err
             });
 
             // Nack all messages in the failed batch
             batchToFlush.forEach((item) => {
                 item.message.nack();
             });
 
-            throw error;
+            throw err;
         }
     }
 
@@ -180,14 +180,16 @@ class BatchWorker {
         this.flushTimer = setTimeout(async () => {
             this.flushTimer = null;
 
+            const pendingBatchSize = this.batch.length;
+            const pendingMessageIds = this.batch.map(item => item.message.id);
             try {
                 await this.flushBatch();
-            } catch (error) {
+            } catch (err) {
                 logger.error({
                     event: 'WorkerScheduledFlushFailed',
-                    batchSize: this.batch.length,
-                    messageIds: this.batch.map(item => item.message.id),
-                    error
+                    batchSize: pendingBatchSize,
+                    messageIds: pendingMessageIds,
+                    err
                 });
             }
 

src/services/events/publisher.ts

@@ -41,15 +41,14 @@ class EventPublisher {
             });
 
             return messageId;
-        } catch (error) {
-            const errorMessage = error instanceof Error ? error.message : String(error);
+        } catch (err) {
             logger.error({
                 event: 'EventPublishFailed',
-                error: errorMessage,
+                err,
                 topic,
                 payload
             });
-            throw error;
+            throw err;
         }
     }
 }

src/services/salt-store/FileSaltStore.ts

@@ -55,8 +55,8 @@ export class FileSaltStore implements ISaltStore {
                 // File doesn't exist, create it with empty object
                 await this.writeFile({});
             }
-        } catch (error) {
-            logger.warn({event: 'FileSaltStoreInitializationFailed', error});
+        } catch (err) {
+            logger.warn({event: 'FileSaltStoreInitializationFailed', err});
         }
     }
 
@@ -80,12 +80,12 @@ export class FileSaltStore implements ISaltStore {
                 }
             }
             return records;
-        } catch (error) {
-            if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
+        } catch (err) {
+            if ((err as NodeJS.ErrnoException).code === 'ENOENT') {
                 return {};
             }
             // Handle corrupted file
-            logger.error({event: 'FileSaltStoreReadFailed', error});
+            logger.error({event: 'FileSaltStoreReadFailed', err});
             await this.writeFile({});
             return {};
         }
@@ -247,9 +247,9 @@ export class FileSaltStore implements ISaltStore {
                 }
 
                 return deletedCount;
-            } catch (error) {
-                logger.error({event: 'FileSaltStoreCleanupFailed', error});
-                throw error;
+            } catch (err) {
+                logger.error({event: 'FileSaltStoreCleanupFailed', err});
+                throw err;
             }
         });
     }

src/services/salt-store/FirestoreSaltStore.ts

@@ -101,9 +101,9 @@ export class FirestoreSaltStore implements ISaltStore {
         try {
             // Simple operation to test connectivity - just get the collection reference
             await this.firestore.collection(this.collectionName).limit(1).get();
-        } catch (error) {
+        } catch (err) {
             // Log warning but don't throw - allow graceful degradation
-            logger.warn({event: 'FirestoreSaltStoreHealthCheckFailed', error});
+            logger.warn({event: 'FirestoreSaltStoreHealthCheckFailed', err});
         }
     }
 
@@ -309,17 +309,17 @@ export class FirestoreSaltStore implements ISaltStore {
                     batchSize
                 });
             }
-        } catch (error) {
+        } catch (err) {
             logger.error({
                 event: 'FirestoreSaltStoreCleanupFailed',
-                error,
+                err,
                 deletedCount: totalDeleted,
                 totalToBeDeleted,
                 durationMs: Date.now() - startTime,
                 cutoffDate: cutoffDate.toISOString(),
                 batchSize
             });
-            throw error;
+            throw err;
         }
 
         logger.info({
@@ -356,21 +356,21 @@ export class FirestoreSaltStore implements ISaltStore {
         try {
             // Use existing set method which handles atomic creation
             return await this.set(key, newSalt);
-        } catch (error) {
+        } catch (err) {
             // Handle race condition: another process created the document between our read and create
             // We could do this with a transaction, but it adds unnecessary performance overhead
-            if (error instanceof SaltAlreadyExistsError) {
+            if (err instanceof SaltAlreadyExistsError) {
                 // Another process created it, read and return the existing document
                 const freshSalt = await this.get(key);
                 if (!freshSalt) {
                     throw new Error(`Document should exist but was not found after race condition for key: ${key}`);
                 }
                 return freshSalt;
             }
-            
+
             // Re-throw other errors
-            logger.error({event: 'FirestoreSaltStoreGetOrCreateFailed', error});
-            throw error;
+            logger.error({event: 'FirestoreSaltStoreGetOrCreateFailed', err});
+            throw err;
         }
     }
 }

src/services/user-signature/UserSignatureService.ts

@@ -38,8 +38,8 @@ export class UserSignatureService {
             try {
                 const deletedCount = await this.saltStore.cleanup();
                 logger.info({event: 'SaltCleanupCompleted', deletedCount});
-            } catch (error) {
-                logger.error({event: 'SaltCleanupFailed', error});
+            } catch (err) {
+                logger.error({event: 'SaltCleanupFailed', err});
             }
         };
 

src/utils/error-formatters.ts

@@ -3,7 +3,7 @@ import {FastifyError, FastifyRequest} from 'fastify';
 export class ErrorDataFormatter {
     static formatValidationError(error: FastifyError, request: FastifyRequest) {
         return {
-            error: {
+            err: {
                 message: error.message,
                 name: error.name,
                 code: error.code,
@@ -21,7 +21,7 @@ export class ErrorDataFormatter {
 
     static formatUnhandledError(error: FastifyError, request: FastifyRequest) {
         return {
-            error: {
+            err: {
                 message: error.message,
                 name: error.name,
                 code: error.code,

test/integration/error-logging.test.ts

@@ -54,7 +54,7 @@ describe('Error Logging', () => {
         // Verify validation error was logged with stack trace and useful request data
         expect(mockWarn).toHaveBeenCalledWith(
             expect.objectContaining({
-                error: expect.objectContaining({
+                err: expect.objectContaining({
                     message: 'querystring must have required property "name"',
                     name: 'FastifyError',
                     code: 'FST_ERR_VALIDATION',
@@ -78,8 +78,8 @@ describe('Error Logging', () => {
 
         // Verify the logged data structure
         const loggedData = mockWarn.mock.calls[0][0];
-        expect(loggedData).toHaveProperty('error.stack');
-        expect(loggedData).toHaveProperty('error.code');
+        expect(loggedData).toHaveProperty('err.stack');
+        expect(loggedData).toHaveProperty('err.code');
         expect(loggedData).toHaveProperty('headers');
         expect(loggedData).toHaveProperty('query');
         expect(loggedData).toHaveProperty('requestBody');
@@ -103,7 +103,7 @@ describe('Error Logging', () => {
         // Verify unhandled error was logged with stack trace and useful request data
         expect(mockError).toHaveBeenCalledWith(
             expect.objectContaining({
-                error: expect.objectContaining({
+                err: expect.objectContaining({
                     message: 'Database connection failed',
                     name: 'DatabaseError',
                     code: 'DATABASE_ERROR',
@@ -127,8 +127,8 @@ describe('Error Logging', () => {
 
         // Verify the logged data structure
         const loggedData = mockError.mock.calls[0][0];
-        expect(loggedData).toHaveProperty('error.stack');
-        expect(loggedData).toHaveProperty('error.code');
+        expect(loggedData).toHaveProperty('err.stack');
+        expect(loggedData).toHaveProperty('err.code');
         expect(loggedData).toHaveProperty('headers');
         expect(loggedData).toHaveProperty('query');
         expect(loggedData).toHaveProperty('requestBody');
@@ -152,8 +152,8 @@ describe('Error Logging', () => {
 
         // Verify the new logging is cleaner and includes stack trace
         const loggedData = mockError.mock.calls[0][0];
-        expect(loggedData).toHaveProperty('error.stack');
-        expect(loggedData).toHaveProperty('error.code');
+        expect(loggedData).toHaveProperty('err.stack');
+        expect(loggedData).toHaveProperty('err.code');
         expect(loggedData).toHaveProperty('headers');
         expect(loggedData).toHaveProperty('query');
         expect(loggedData).toHaveProperty('requestBody');