fix: increase writeAuditEntry 412 retries to 5 and remove dead retry loop in put.js

src/storage/version/audit.js

@@ -191,13 +191,14 @@ function usersNormalized(usersJson) {
  * and within AUDIT_TIME_WINDOW_MS and both last and new are edits (no version), replace that
  * line; else append. A version entry always appends and is never replaced (breaks the window).
  *
- * Uses If-Match on the PUT so that a concurrent write causes a 412, which triggers one retry.
+ * Uses If-Match on the PUT so that a concurrent write causes a 412, which triggers up to 4
+ * retries with random jitter to reduce thundering-herd contention (5 total attempts).
  * @param {object} env
  * @param {{ bucket: string, org: string }} ctx - bucket, org
  * @param {string} repo
  * @param {string} fileId
  * @param {object} entry - { timestamp, users, path, versionLabel?, versionId? }
- * @param {number} [attempt=0] - retry counter (max 1 retry)
+ * @param {number} [attempt=0] - retry counter (max 4 retries)
  * @returns {Promise<{ status: number }>}
  */
 export async function writeAuditEntry(env, ctx, repo, fileId, entry, attempt = 0) {
@@ -262,16 +263,19 @@ export async function writeAuditEntry(env, ctx, repo, fileId, entry, attempt = 0
       Body: newContent,
       ContentType: 'text/plain; charset=utf-8',
     };
-    // Guard against concurrent writes: if someone else wrote since our GET, the PUT
-    // will fail with 412 and we retry once with a fresh read.
     if (etag) putInput.IfMatch = etag;
 
     try {
       const resp = await client.send(new PutObjectCommand(putInput));
       return { status: resp?.$metadata?.httpStatusCode ?? 200 };
     } catch (e) {
-      if (e?.$metadata?.httpStatusCode === 412 && attempt === 0) {
-        return writeAuditEntry(env, ctx, repo, fileId, entry, 1);
+      if (e?.$metadata?.httpStatusCode === 412 && attempt < 4) {
+        const delay = Math.random() * 50 * (attempt + 1);
+        // eslint-disable-next-line no-await-in-loop -- sequential retry with jitter
+        await new Promise((r) => {
+          setTimeout(r, delay);
+        });
+        return writeAuditEntry(env, ctx, repo, fileId, entry, attempt + 1);
       }
       throw e;
     }

src/storage/version/put.js

@@ -23,8 +23,6 @@ import getObject from '../object/get.js';
 import { writeAuditEntry } from './audit.js';
 import { versionKey } from './paths.js';
 
-const AUDIT_WRITE_RETRIES = 3;
-
 export function getContentLength(body) {
   if (body === undefined) {
     return undefined;
@@ -255,25 +253,13 @@ export async function putObjectWithVersion(
     const pathForAudit = (daCtx.site && Path.startsWith(`${daCtx.site}/`))
       ? Path.slice(daCtx.site.length)
       : Path;
-    let auditErr;
-    for (let i = 0; i < AUDIT_WRITE_RETRIES; i += 1) {
-      try {
-        // eslint-disable-next-line no-await-in-loop
-        await writeAuditEntry(env, { bucket: input.Bucket, org: daCtx.org }, daCtx.site, ID, {
-          timestamp: Timestamp,
-          users: Users,
-          path: pathForAudit,
-          versionLabel,
-          versionId,
-        });
-        auditErr = null;
-        break;
-      } catch (e) { auditErr = e; }
-    }
-    if (auditErr) {
-      // eslint-disable-next-line no-console
-      console.error(`Failed to write audit entry after ${AUDIT_WRITE_RETRIES} retries`, auditErr);
-    }
+    await writeAuditEntry(env, { bucket: input.Bucket, org: daCtx.org }, daCtx.site, ID, {
+      timestamp: Timestamp,
+      users: Users,
+      path: pathForAudit,
+      versionLabel,
+      versionId,
+    });
   }
 
   const metadata = {

test/storage/object/conditionals.test.js

@@ -262,8 +262,8 @@ describe('Conditional Headers', () => {
 
       // Should return 412 and NOT retry
       assert.strictEqual(resp.status, 412);
-      // 2 audit PUT attempts (original + 1 retry on 412) + 1 main PUT = 3 total
-      assert.strictEqual(s3Mock.commandCalls(PutObjectCommand).length, 3);
+      // 5 audit PUT attempts (1 initial + 4 retries on 412) + 1 main PUT = 6 total
+      assert.strictEqual(s3Mock.commandCalls(PutObjectCommand).length, 6);
     });
   });
 

test/storage/version/audit.test.js

@@ -644,7 +644,7 @@ describe('Version Audit', () => {
       assert.strictEqual(putCalls[0].IfMatch, undefined, 'If-Match must be absent for first write');
     });
 
-    it('retries once on 412 from PUT and succeeds on second attempt', async () => {
+    it('retries on 412 from PUT and succeeds on a later attempt', async () => {
       let getCallCount = 0;
       const putCalls = [];
 
@@ -667,8 +667,8 @@ describe('Version Audit', () => {
                 }
                 if (cmd instanceof PutObjectCommand) {
                   putCalls.push(cmd.input);
-                  if (putCalls.length === 1) {
-                    // First PUT: simulate concurrent write → 412
+                  if (putCalls.length < 3) {
+                    // First two PUTs: simulate concurrent write → 412
                     const err = new Error('precondition failed');
                     err.$metadata = { httpStatusCode: 412 };
                     throw err;
@@ -692,10 +692,11 @@ describe('Version Audit', () => {
       });
 
       assert.strictEqual(result.status, 200);
-      assert.strictEqual(getCallCount, 2, 'must re-read on retry');
-      assert.strictEqual(putCalls.length, 2, 'must retry the PUT');
+      assert.strictEqual(getCallCount, 3, 'must re-read on each retry');
+      assert.strictEqual(putCalls.length, 3, 'must retry the PUT until success');
       assert.strictEqual(putCalls[0].IfMatch, '"etag-1"');
-      assert.strictEqual(putCalls[1].IfMatch, '"etag-2"', 'retry uses fresh ETag');
+      assert.strictEqual(putCalls[1].IfMatch, '"etag-2"', 'first retry uses fresh ETag');
+      assert.strictEqual(putCalls[2].IfMatch, '"etag-3"', 'second retry uses fresh ETag');
     });
 
     it('archives existing content and starts fresh when entry count reaches AUDIT_MAX_ENTRIES', async () => {
@@ -813,7 +814,10 @@ describe('Version Audit', () => {
       assert.strictEqual(lines.length, 2, 'both entries must be present (no collapse across mismatched users)');
     });
 
-    it('returns status 500 when PUT 412 on retry attempt (no further retries)', async () => {
+    it('returns status 500 when PUT 412 persists across all 5 attempts', async () => {
+      let getCallCount = 0;
+      let putCallCount = 0;
+
       const makeBody = () => new ReadableStream({
         start(controller) {
           controller.enqueue(new TextEncoder().encode(''));
@@ -828,9 +832,11 @@ describe('Version Audit', () => {
             S3Client: function S3Client() {
               this.send = async (cmd) => {
                 if (cmd instanceof GetObjectCommand) {
+                  getCallCount += 1;
                   return { Body: makeBody(), ETag: '"etag-x"' };
                 }
                 if (cmd instanceof PutObjectCommand) {
+                  putCallCount += 1;
                   const err = new Error('precondition failed');
                   err.$metadata = { httpStatusCode: 412 };
                   throw err;
@@ -851,8 +857,61 @@ describe('Version Audit', () => {
         path: 'repo/doc.html',
       });
 
-      assert.strictEqual(result.status, 500, 'persistent 412 must surface as 500 after one retry');
+      assert.strictEqual(result.status, 500, 'persistent 412 must surface as 500 after 5 total attempts');
       assert.strictEqual(result.error, 'precondition failed');
+      assert.strictEqual(putCallCount, 5, 'must attempt PUT 5 times total (1 initial + 4 retries)');
+      assert.strictEqual(getCallCount, 5, 'must re-read on each attempt');
+    });
+
+    it('succeeds on the 5th attempt after four 412s', async () => {
+      let getCallCount = 0;
+      let putCallCount = 0;
+
+      const makeBody = () => new ReadableStream({
+        start(controller) {
+          controller.enqueue(new TextEncoder().encode(''));
+          controller.close();
+        },
+      });
+
+      const { writeAuditEntry } = await esmock(
+        '../../../src/storage/version/audit.js',
+        {
+          '@aws-sdk/client-s3': {
+            S3Client: function S3Client() {
+              this.send = async (cmd) => {
+                if (cmd instanceof GetObjectCommand) {
+                  getCallCount += 1;
+                  return { Body: makeBody(), ETag: `"etag-${getCallCount}"` };
+                }
+                if (cmd instanceof PutObjectCommand) {
+                  putCallCount += 1;
+                  if (putCallCount < 5) {
+                    const err = new Error('precondition failed');
+                    err.$metadata = { httpStatusCode: 412 };
+                    throw err;
+                  }
+                  return { $metadata: { httpStatusCode: 200 } };
+                }
+                return { $metadata: { httpStatusCode: 200 } };
+              };
+            },
+            GetObjectCommand,
+            PutObjectCommand,
+          },
+          '../../../src/storage/utils/config.js': { default: () => ({}) },
+        },
+      );
+
+      const result = await writeAuditEntry({}, { bucket: 'b', org: 'o' }, 'repo', 'fid', {
+        timestamp: '5000',
+        users: '[{"email":"u@x.com"}]',
+        path: 'repo/doc.html',
+      });
+
+      assert.strictEqual(result.status, 200, 'must succeed on the 5th attempt');
+      assert.strictEqual(putCallCount, 5, 'must have attempted PUT 5 times');
+      assert.strictEqual(getCallCount, 5, 'must re-read on each attempt');
     });
   });
 });