diff --git a/package-lock.json b/package-lock.json index 65367bcd..8f6ecf8e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -448,6 +448,7 @@ "resolved": "https://registry.npmjs.org/@aws-sdk/client-sso-oidc/-/client-sso-oidc-3.726.0.tgz", "integrity": "sha512-5JzTX9jwev7+y2Jkzjz0pd1wobB5JQfPOQF3N2DrJ5Pao0/k6uRYwE4NqB0p0HlGrMTDm7xNq7OSPPIPG575Jw==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@aws-crypto/sha256-browser": "5.2.0", "@aws-crypto/sha256-js": "5.2.0", @@ -501,6 +502,7 @@ "resolved": "https://registry.npmjs.org/@aws-sdk/client-sts/-/client-sts-3.726.1.tgz", "integrity": "sha512-qh9Q9Vu1hrM/wMBOBIaskwnE4GTFaZu26Q6WHwyWNfj7J8a40vBxpW16c2vYXHLBtwRKM1be8uRLkmDwghpiNw==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@aws-crypto/sha256-browser": "5.2.0", "@aws-crypto/sha256-js": "5.2.0", @@ -1381,7 +1383,8 @@ "version": "4.20260101.0", "resolved": "https://registry.npmjs.org/@cloudflare/workers-types/-/workers-types-4.20260101.0.tgz", "integrity": "sha512-C28o4U1T4dPe8avLv1xMQ4MtSE6G4skbVA3VfVZIrpTqklvO+homKB1uFEtNbuZlaKg+Znv8sjmGQUInTH17gA==", - "license": "MIT OR Apache-2.0" + "license": "MIT OR Apache-2.0", + "peer": true }, "node_modules/@colors/colors": { "version": "1.6.0", @@ -2709,6 +2712,7 @@ "integrity": "sha512-DhGl4xMVFGVIyMwswXeyzdL4uXD5OGILGX5N8Y+f6W7LhC1Ze2poSNrkF/fedpVDHEEZ+PHFW0vL14I+mm8K3Q==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@octokit/auth-token": "^6.0.0", "@octokit/graphql": "^9.0.3", @@ -2855,6 +2859,7 @@ "integrity": "sha512-3giAOQvZiH5F9bMlMiv8+GSPMeqg0dbaeo58/0SlA9sxSqZhnUtxzX9/2FzyhS9sWQf5S0GJE0AKBrFqjpeYcg==", "dev": true, "license": "Apache-2.0", + "peer": true, "engines": { "node": ">=8.0.0" } @@ -4992,6 +4997,7 @@ "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "dev": true, "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -5049,6 +5055,7 @@ "integrity": "sha512-B/gBuNg5SiMTrPkC+A2+cW0RszwxYmn6VYxB/inlBStS5nx6xHIt/ehKRhIMhqusl7a8LjQoZnjCs5vhwxOQ1g==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "fast-deep-equal": "^3.1.3", "fast-uri": "^3.0.1", @@ -7353,6 +7360,7 @@ "integrity": "sha512-sjc7Y8cUD1IlwYcTS9qPSvGjAC8Ne9LctpxKKu3x/1IC9bnOg98Zy6GxEJUfr1NojMgVPlyANXYns8oE2c1TAA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.2.0", "@eslint-community/regexpp": "^4.6.1", @@ -10397,6 +10405,7 @@ "integrity": "sha512-PRsaiG84bK+AMvxziE/lCFss8juXjNaWzVbN5tXAm4XjeaS9NAHhop+PjQxz2A9h8Q4M/xGmzP8vqNwy6JeK0A==", "dev": true, "license": "MIT", + "peer": true, "bin": { "marked": "bin/marked.js" }, @@ -10947,6 +10956,7 @@ "integrity": "sha512-UczzB+0nnwGotYSgllfARAqWCJ5e/skuV2K/l+Zyck/H6pJIhLXuBnz+6vn2i211o7DtbE78HQtsYEKICHGI+g==", "dev": true, "license": "MIT", + "peer": true, "funding": { "type": "opencollective", "url": "https://opencollective.com/mobx" @@ -13502,6 +13512,7 @@ "dev": true, "inBundle": true, "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -14617,6 +14628,7 @@ "integrity": "sha512-DGrYcCWK7tvYMnWh79yrPHt+vdx9tY+1gPZa7nJQtO/p8bLTDaHp4dzwEhQB7pZ4Xe3ok4XKuEPrVuc+wlpkmw==", "dev": true, "license": "MIT", + "peer": true, "engines": { "node": ">=0.10.0" } @@ -14627,6 +14639,7 @@ "integrity": "sha512-ibrK8llX2a4eOskq1mXKu/TGZj9qzomO+sNfO98M6d9zIPOEhlBkMkBUBLd1vgS0gQsLDBzA+8jJBVXDnfHmJg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "scheduler": "^0.27.0" }, @@ -15240,6 +15253,7 @@ "integrity": "sha512-6qGjWccl5yoyugHt3jTgztJ9Y0JVzyH8/Voc/D8PlLat9pwxQYXz7W1Dpnq5h0/G5GCYGUaDSlYcyk3AMh5A6g==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@semantic-release/commit-analyzer": "^13.0.1", "@semantic-release/error": "^4.0.0", @@ -16618,6 +16632,7 @@ "integrity": "sha512-1v/e3Dl1BknC37cXMhwGomhO8AkYmN41CqyX9xhUDxry1ns3BFQy2lLDRQXJRdVVWB9OHemv/53xaStimvWyuA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@emotion/is-prop-valid": "1.2.2", "@emotion/unitless": "0.8.1", @@ -17349,6 +17364,7 @@ "integrity": "sha512-i7qRCmY42zmCwnYlh9H2SvLEypEFGye5iRmEMKjcGi7zk9UquigRjFtTLz0TYqr0ZGLZhaMHl/foy1bZR+Cwlw==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "pathe": "^2.0.3" } @@ -17802,6 +17818,7 @@ "dev": true, "hasInstallScript": true, "license": "Apache-2.0", + "peer": true, "bin": { "workerd": "bin/workerd" }, diff --git a/src/storage/version/audit.js b/src/storage/version/audit.js index 7d58f421..c51332b7 100644 --- a/src/storage/version/audit.js +++ b/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; } diff --git a/src/storage/version/put.js b/src/storage/version/put.js index aa1d5de4..8af5de60 100644 --- a/src/storage/version/put.js +++ b/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 = { diff --git a/test/storage/object/conditionals.test.js b/test/storage/object/conditionals.test.js index c9223a62..d5efe35e 100644 --- a/test/storage/object/conditionals.test.js +++ b/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); }); }); diff --git a/test/storage/version/audit.test.js b/test/storage/version/audit.test.js index f7f0d851..3e8f2d7f 100644 --- a/test/storage/version/audit.test.js +++ b/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'); }); }); }); diff --git a/test/storage/version/put.test.js b/test/storage/version/put.test.js index 7d16ef76..33dba6f5 100644 --- a/test/storage/version/put.test.js +++ b/test/storage/version/put.test.js @@ -2554,11 +2554,11 @@ describe('Version Put', () => { ); }); - it('retries writeAuditEntry on transient failure and succeeds on second attempt', async () => { + it('calls writeAuditEntry once and succeeds (retries are handled inside writeAuditEntry)', async () => { let callCount = 0; const mockWriteAuditEntry = async () => { callCount += 1; - if (callCount < 2) throw new Error('transient R2 error'); + return { status: 200 }; }; const mockS3Client = { send: () => ({ $metadata: { httpStatusCode: 200 } }) }; @@ -2589,15 +2589,15 @@ describe('Version Put', () => { true, ); - assert.strictEqual(resp.status, 200, 'document write must succeed despite transient audit error'); - assert.strictEqual(callCount, 2, 'writeAuditEntry must be retried once after first failure'); + assert.strictEqual(resp.status, 200, 'document write must succeed'); + assert.strictEqual(callCount, 1, 'put.js must call writeAuditEntry exactly once (retries are handled inside writeAuditEntry)'); }); - it('logs error and continues when writeAuditEntry fails all retries', async () => { + it('writeAuditEntry returning status 500 does not affect main put result', async () => { let callCount = 0; const mockWriteAuditEntry = async () => { callCount += 1; - throw new Error('persistent R2 error'); + return { status: 500, error: 'persistent R2 error' }; }; const mockS3Client = { send: () => ({ $metadata: { httpStatusCode: 200 } }) }; @@ -2617,31 +2617,19 @@ describe('Version Put', () => { '../../../src/storage/version/audit.js': { writeAuditEntry: mockWriteAuditEntry }, }); - const errors = []; - const origError = console.error; - console.error = (...args) => errors.push(args.map(String).join(' ')); - let resp; - try { - resp = await putObjectWithVersion( - {}, - { - org: 'o', ext: 'html', site: 'repo', users: [], - }, - { - org: 'o', key: 'repo/p.html', body: 'edit', type: 'text/html', - }, - true, - ); - } finally { - console.error = origError; - } - - assert.strictEqual(resp.status, 200, 'document write must succeed even when audit write is permanently failing'); - assert.strictEqual(callCount, 3, 'writeAuditEntry must be attempted 3 times before giving up'); - assert.ok( - errors.some((e) => e.includes('after 3 retries')), - 'error after all retries exhausted must be logged with retry count', + const resp = await putObjectWithVersion( + {}, + { + org: 'o', ext: 'html', site: 'repo', users: [], + }, + { + org: 'o', key: 'repo/p.html', body: 'edit', type: 'text/html', + }, + true, ); + + assert.strictEqual(resp.status, 200, 'document write must succeed even when audit returns 500'); + assert.strictEqual(callCount, 1, 'writeAuditEntry must be called exactly once'); }); it('does not write audit for non-versionable type (e.g. PDF)', async () => { @@ -2888,7 +2876,7 @@ describe('Version Put', () => { assert.strictEqual(resp.metadata.id, 'file-id-err'); }); - it('swallows writeAuditEntry error and still returns main put result', async () => { + it('writeAuditEntry returning status 500 does not prevent main put result', async () => { const mockGetObject = async () => ({ status: 200, body: 'doc', @@ -2910,7 +2898,7 @@ describe('Version Put', () => { ifMatch: () => s3Client, }, '../../../src/storage/version/audit.js': { - writeAuditEntry: async () => { throw new Error('audit service unavailable'); }, + writeAuditEntry: async () => ({ status: 500, error: 'audit service unavailable' }), }, }); @@ -2920,7 +2908,6 @@ describe('Version Put', () => { const update = { org: 'o', key: 'repo/doc.html', type: 'text/html' }; const resp = await putObjectWithVersion({}, daCtx, update, true); - // audit failure must not bubble up; main put succeeded assert.strictEqual(resp.status, 200); assert.strictEqual(resp.metadata.id, 'audit-err-id'); });