From bec1a1cd6d2c7d5b1d385ef354a3ded6b5728eec Mon Sep 17 00:00:00 2001 From: kptdobe Date: Thu, 23 Apr 2026 11:56:05 +0200 Subject: [PATCH] feat: add a retry to compensate R2 transient failure --- src/storage/version/put.js | 39 ++++++++------ test/storage/version/put.test.js | 90 ++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 15 deletions(-) diff --git a/src/storage/version/put.js b/src/storage/version/put.js index c8b9dab9..d4786d70 100644 --- a/src/storage/version/put.js +++ b/src/storage/version/put.js @@ -23,6 +23,8 @@ import getObject from '../object/get.js'; import { writeAuditEntry } from './audit.js'; import { versionKeyNew, versionKeyLegacy } from './paths.js'; +const AUDIT_WRITE_RETRIES = 3; + export function getContentLength(body) { if (body === undefined) { return undefined; @@ -246,22 +248,29 @@ export async function putObjectWithVersion( // Store path without repo prefix and versionId without extension for readability. if (createVersion) { if (usesAuditFile) { - try { - const versionId = versionCreated ? Version : undefined; - const versionLabel = versionCreated ? (Label ?? '') : undefined; - const pathForAudit = (daCtx.site && Path.startsWith(`${daCtx.site}/`)) - ? Path.slice(daCtx.site.length) - : Path; - await writeAuditEntry(env, { bucket: input.Bucket, org: daCtx.org }, daCtx.site, ID, { - timestamp: Timestamp, - users: Users, - path: pathForAudit, - versionLabel, - versionId, - }); - } catch (e) { + const versionId = versionCreated ? Version : undefined; + const versionLabel = versionCreated ? (Label ?? '') : undefined; + 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', e); + console.error(`Failed to write audit entry after ${AUDIT_WRITE_RETRIES} retries`, auditErr); } } else if (!shouldCreateVersionObject) { // Legacy path: write an empty version object so listFromLegacyStructure can find it. diff --git a/test/storage/version/put.test.js b/test/storage/version/put.test.js index e50061e4..723ca1dc 100644 --- a/test/storage/version/put.test.js +++ b/test/storage/version/put.test.js @@ -2534,6 +2534,96 @@ describe('Version Put', () => { ); }); + it('retries writeAuditEntry on transient failure and succeeds on second attempt', async () => { + let callCount = 0; + const mockWriteAuditEntry = async () => { + callCount += 1; + if (callCount < 2) throw new Error('transient R2 error'); + }; + + const mockS3Client = { send: () => ({ $metadata: { httpStatusCode: 200 } }) }; + const mockGetObject = async () => ({ + body: 'content', + contentType: 'text/html', + metadata: { id: 'doc-id', version: 'v1' }, + status: 200, + }); + + const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', { + '../../../src/storage/object/get.js': { default: mockGetObject }, + '../../../src/storage/utils/version.js': { + ifNoneMatch: () => mockS3Client, + ifMatch: () => mockS3Client, + }, + '../../../src/storage/version/audit.js': { writeAuditEntry: mockWriteAuditEntry }, + }); + + const resp = await putObjectWithVersion( + { VERSIONS_AUDIT_FILE_ORGS: 'o' }, + { + 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 despite transient audit error'); + assert.strictEqual(callCount, 2, 'writeAuditEntry must be retried once after first failure'); + }); + + it('logs error and continues when writeAuditEntry fails all retries', async () => { + let callCount = 0; + const mockWriteAuditEntry = async () => { + callCount += 1; + throw new Error('persistent R2 error'); + }; + + const mockS3Client = { send: () => ({ $metadata: { httpStatusCode: 200 } }) }; + const mockGetObject = async () => ({ + body: 'content', + contentType: 'text/html', + metadata: { id: 'doc-id', version: 'v1' }, + status: 200, + }); + + const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', { + '../../../src/storage/object/get.js': { default: mockGetObject }, + '../../../src/storage/utils/version.js': { + ifNoneMatch: () => mockS3Client, + ifMatch: () => mockS3Client, + }, + '../../../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( + { VERSIONS_AUDIT_FILE_ORGS: 'o' }, + { + 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', + ); + }); + it('writes legacy empty version object when org is not in VERSIONS_AUDIT_FILE_ORGS', async () => { const auditCalls = []; const legacyPutCalls = [];