retain locks

Author: alexnuttall

lambdas/sftp-letters/src/__tests__/app/send.test.ts

@@ -239,8 +239,8 @@ describe('App', () => {
       `${baseUploadDir}/${sftpEnvironment}/batches/${templateId}/${batchId}_MANIFEST.csv`
     );
 
-    expect(mocks.templateRepository.clearLock).toHaveBeenCalledTimes(1);
-    expect(mocks.templateRepository.clearLock).toHaveBeenCalledWith(
+    expect(mocks.templateRepository.finaliseLock).toHaveBeenCalledTimes(1);
+    expect(mocks.templateRepository.finaliseLock).toHaveBeenCalledWith(
       owner,
       templateId
     );
@@ -355,10 +355,10 @@ describe('App', () => {
     expect(mocks.sftpClient.exists).not.toHaveBeenCalled();
     expect(mocks.sftpClient.mkdir).not.toHaveBeenCalled();
     expect(mocks.sftpClient.put).not.toHaveBeenCalled();
-    expect(mocks.templateRepository.clearLock).not.toHaveBeenCalled();
+    expect(mocks.templateRepository.finaliseLock).not.toHaveBeenCalled();
   });
 
-  test('exits early and does not send if the manifest is already in the SFTP server, clears existing lock', async () => {
+  test('exits early and does not send if the manifest is already in the SFTP server, finalises existing lock', async () => {
     const { app, mocks, logMessages } = setup();
 
     const personalisationParameters = ['pdsField'];
@@ -442,8 +442,8 @@ describe('App', () => {
       `${baseUploadDir}/${sftpEnvironment}/batches/${templateId}/${batchId}_MANIFEST.csv`
     );
 
-    expect(mocks.templateRepository.clearLock).toHaveBeenCalledTimes(1);
-    expect(mocks.templateRepository.clearLock).toHaveBeenCalledWith(
+    expect(mocks.templateRepository.finaliseLock).toHaveBeenCalledTimes(1);
+    expect(mocks.templateRepository.finaliseLock).toHaveBeenCalledWith(
       owner,
       templateId
     );

lambdas/sftp-letters/src/__tests__/infra/template-lock-repository.test.ts

@@ -41,12 +41,13 @@ describe('TemplateLockRepository', () => {
           '#sftpSendLockTime': 'sftpSendLockTime',
         },
         ExpressionAttributeValues: {
-          ':updatedAt': expect.stringMatching(isoDateRegExp),
+          ':condition_1_sftpSendLockTime': 0,
           ':condition_2_sftpSendLockTime': mockDate.getTime() + sendLockTtlMs,
+          ':updatedAt': expect.stringMatching(isoDateRegExp),
           ':sftpSendLockTime': mockDate.getTime(),
         },
         ConditionExpression:
-          'attribute_not_exists (#sftpSendLockTime) OR #sftpSendLockTime > :condition_2_sftpSendLockTime',
+          '#sftpSendLockTime <> :condition_1_sftpSendLockTime AND #sftpSendLockTime > :condition_2_sftpSendLockTime OR attribute_not_exists (#sftpSendLockTime)',
         Key: {
           id: templateId,
           owner,
@@ -83,28 +84,30 @@ describe('TemplateLockRepository', () => {
     });
   });
 
-  describe('clearLock', () => {
-    test('removes lock attribute', async () => {
+  describe('finaliseLock', () => {
+    test('unconditionally sets lockTime to zero', async () => {
       const { mocks, templateRepository } = setup();
 
       mocks.client.on(UpdateCommand).resolvesOnce({});
 
-      await templateRepository.clearLock(owner, templateId);
+      await templateRepository.finaliseLock(owner, templateId);
+
       expect(mocks.client).toHaveReceivedCommandWith(UpdateCommand, {
         ExpressionAttributeNames: {
-          '#sftpSendLockTime': 'sftpSendLockTime',
           '#updatedAt': 'updatedAt',
+          '#sftpSendLockTime': 'sftpSendLockTime',
         },
         ExpressionAttributeValues: {
           ':updatedAt': expect.stringMatching(isoDateRegExp),
+          ':sftpSendLockTime': 0,
         },
         Key: {
           id: templateId,
           owner,
         },
         TableName: templatesTableName,
         UpdateExpression:
-          'SET #updatedAt = :updatedAt REMOVE #sftpSendLockTime',
+          'SET #sftpSendLockTime = :sftpSendLockTime, #updatedAt = :updatedAt',
       });
     });
   });

lambdas/sftp-letters/src/app/send.ts

@@ -75,7 +75,7 @@ export class App {
         templateLogger.warn(
           'Manifest already exists, assuming duplicate event'
         );
-        await this.templateRepository.clearLock(owner, templateId);
+        await this.templateRepository.finaliseLock(owner, templateId);
         return 'already-sent';
       }
 
@@ -97,7 +97,7 @@ export class App {
 
       templateLogger.info('Removing lock');
 
-      await this.templateRepository.clearLock(owner, templateId);
+      await this.templateRepository.finaliseLock(owner, templateId);
 
       templateLogger.info('Sent proofing request');
 

lambdas/sftp-letters/src/infra/template-lock-repository.ts

@@ -14,7 +14,7 @@ export class TemplateLockRepository {
     const time = this.getDate().getTime();
 
     const update = new TemplateUpdateBuilder(this.templatesTableName, owner, id)
-      .setLockTime('sftpSendLockTime', time, time + this.lockTtl)
+      .setLockTimeConditionally('sftpSendLockTime', time, time + this.lockTtl)
       .build();
 
     try {
@@ -29,9 +29,9 @@ export class TemplateLockRepository {
     return true;
   }
 
-  async clearLock(owner: string, id: string) {
+  async finaliseLock(owner: string, id: string) {
     const update = new TemplateUpdateBuilder(this.templatesTableName, owner, id)
-      .removeLockTime('sftpSendLockTime')
+      .setLockTimeUnconditionally('sftpSendLockTime', 0)
       .build();
 
     return await this.client.send(new UpdateCommand(update));

utils/entity-update-command-builder/src/__tests__/template-update-builder.test.ts

@@ -167,25 +167,29 @@ describe('TemplateBuilder', () => {
     });
   });
 
-  describe('setLockTime', () => {
-    test('sets lock time if no lock exists', () => {
+  describe('setLockTimeConditionally', () => {
+    test('sets lock time if no lock exists, or lock is not finalised (set to zero)', () => {
       const builder = new TemplateUpdateBuilder(
         mockTableName,
         mockOwner,
         mockId
       );
 
-      const res = builder.setLockTime('sftpSendLockTime', 500).build();
+      const res = builder
+        .setLockTimeConditionally('sftpSendLockTime', 500)
+        .build();
 
       expect(res).toEqual({
-        ConditionExpression: 'attribute_not_exists (#sftpSendLockTime)',
+        ConditionExpression:
+          '#sftpSendLockTime <> :condition_1_sftpSendLockTime OR attribute_not_exists (#sftpSendLockTime)',
         ExpressionAttributeNames: {
           '#sftpSendLockTime': 'sftpSendLockTime',
           '#updatedAt': 'updatedAt',
         },
         ExpressionAttributeValues: {
           ':sftpSendLockTime': 500,
           ':updatedAt': '2025-01-01T09:00:00.000Z',
+          ':condition_1_sftpSendLockTime': 0,
         },
         Key: {
           id: 'Hello2',
@@ -204,16 +208,19 @@ describe('TemplateBuilder', () => {
         mockId
       );
 
-      const res = builder.setLockTime('sftpSendLockTime', 1000, 1500).build();
+      const res = builder
+        .setLockTimeConditionally('sftpSendLockTime', 1000, 1500)
+        .build();
 
       expect(res).toEqual({
         ConditionExpression:
-          'attribute_not_exists (#sftpSendLockTime) OR #sftpSendLockTime > :condition_2_sftpSendLockTime',
+          '#sftpSendLockTime <> :condition_1_sftpSendLockTime AND #sftpSendLockTime > :condition_2_sftpSendLockTime OR attribute_not_exists (#sftpSendLockTime)',
         ExpressionAttributeNames: {
           '#sftpSendLockTime': 'sftpSendLockTime',
           '#updatedAt': 'updatedAt',
         },
         ExpressionAttributeValues: {
+          ':condition_1_sftpSendLockTime': 0,
           ':condition_2_sftpSendLockTime': 1500,
           ':sftpSendLockTime': 1000,
           ':updatedAt': '2025-01-01T09:00:00.000Z',
@@ -229,22 +236,25 @@ describe('TemplateBuilder', () => {
     });
   });
 
-  describe('removeLockTime', () => {
-    test('clears lockTime attribute', () => {
+  describe('setLockTimeUnconditionally', () => {
+    test('sets lock time without conditions', () => {
       const builder = new TemplateUpdateBuilder(
         mockTableName,
         mockOwner,
         mockId
       );
 
-      const res = builder.removeLockTime('sftpSendLockTime').build();
+      const res = builder
+        .setLockTimeUnconditionally('sftpSendLockTime', 500)
+        .build();
 
       expect(res).toEqual({
         ExpressionAttributeNames: {
           '#sftpSendLockTime': 'sftpSendLockTime',
           '#updatedAt': 'updatedAt',
         },
         ExpressionAttributeValues: {
+          ':sftpSendLockTime': 500,
           ':updatedAt': '2025-01-01T09:00:00.000Z',
         },
         Key: {
@@ -253,7 +263,7 @@ describe('TemplateBuilder', () => {
         },
         TableName: 'TABLE_NAME',
         UpdateExpression:
-          'SET #updatedAt = :updatedAt REMOVE #sftpSendLockTime',
+          'SET #sftpSendLockTime = :sftpSendLockTime, #updatedAt = :updatedAt',
       });
     });
   });

utils/entity-update-command-builder/src/template-update-builder.ts

@@ -34,24 +34,32 @@ export class TemplateUpdateBuilder extends EntityUpdateBuilder<MergedTemplate> {
     return this;
   }
 
-  setLockTime(
+  setLockTimeConditionally(
     lockField: 'sftpSendLockTime',
     timeMs: number,
     lockExpiryTimeMs?: number
   ) {
     this.updateBuilder
       .setValue(lockField, timeMs)
-      .fnCondition(lockField, null, 'attribute_not_exists');
+      .andCondition(lockField, 0, '<>');
 
     if (lockExpiryTimeMs) {
-      this.updateBuilder.orCondition(lockField, lockExpiryTimeMs, '>');
+      this.updateBuilder.andCondition(lockField, lockExpiryTimeMs, '>');
     }
 
+    this.updateBuilder.fnCondition(
+      lockField,
+      null,
+      'attribute_not_exists',
+      false,
+      'OR'
+    );
+
     return this;
   }
 
-  removeLockTime(lockField: 'sftpSendLockTime') {
-    this.updateBuilder.removeAttribute(lockField);
+  setLockTimeUnconditionally(lockField: 'sftpSendLockTime', timeMs: number) {
+    this.updateBuilder.setValue(lockField, timeMs);
 
     return this;
   }