Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions src/storage/version/put.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,18 +295,15 @@ export async function putObjectWithVersion(
} catch (e) {
const status = e.$metadata?.httpStatusCode || 500;
if (status === 412) {
// Only retry if no client conditionals (internal operation) and under retry limit
if (!effectiveConditionals?.ifMatch) {
return putObjectWithVersion(
env,
daCtx,
update,
body,
guid,
clientConditionals,
);
// Retry when no client conditional (internal operation) or when the client sent
// If-Match: * (wildcard — asserts existence only, already verified above; a concurrent
// write changed the ETag, so re-fetch and retry like the no-conditional path).
// A specific ETag means the client explicitly requires that version, so propagate 412.
const shouldRetry = !effectiveConditionals?.ifMatch
|| effectiveConditionals.ifMatch === '*';
if (shouldRetry) {
return putObjectWithVersion(env, daCtx, update, body, guid, clientConditionals);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old code mentions "retry limit" though I don't see code for that. Seems like there should be a limit otherwise it might be possible to stack overflow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I do not really understand the original code.

I do not think the "under retry limit" was there before. No behavior change but it is worth understanding why.

@karlpauls could you please review and comment ?

}
// Client conditional failed or max retries exceeded, return 412
return { status: 412, metadata: { id: ID } };
}

Expand Down
80 changes: 80 additions & 0 deletions test/storage/version/put.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,86 @@ describe('Version Put', () => {
assert.strictEqual(resp.error, 'testing 123');
});

it('putObjectWithVersion retries on ETag mismatch when If-Match: * is sent (existing document, concurrent write)', async () => {
let getObjectCallCount = 0;
const mockGetObject = async () => {
getObjectCallCount += 1;
return { status: 200, metadata: { id: 'existing-id' }, etag: `etag-${getObjectCallCount}` };
};

let firstWrite = true;
const sendCalls = [];
const mockIfMatchClient = {
async send(cmd) {
sendCalls.push(cmd);
if (firstWrite) {
firstWrite = false;
const err = { $metadata: { httpStatusCode: 412 } };
throw err;
}
return { $metadata: { httpStatusCode: 200 } };
},
};

const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', {
'../../../src/storage/object/get.js': { default: mockGetObject },
'../../../src/storage/utils/version.js': {
ifMatch: () => mockIfMatchClient,
ifNoneMatch: () => mockIfMatchClient,
},
});

const resp = await putObjectWithVersion(
{ env: true },
{ users: [{ email: 'a@b.com' }] },
{ org: 'org', key: 'doc.html', type: 'text/html' },
null,
null,
{ ifMatch: '*' },
);

assert.equal(200, resp.status, 'Should succeed after retry');
assert.equal(2, getObjectCallCount, 'Should have fetched current state twice (initial + retry)');
assert.equal(2, sendCalls.length, 'Should have attempted R2 write twice');
});

it('putObjectWithVersion returns 412 without retry when client sends specific ETag and R2 rejects it', async () => {
const mockGetObject = async () => ({
status: 200,
metadata: { id: 'existing-id' },
etag: '"server-etag"',
});

const sendCalls = [];
const mockIfMatchClient = {
async send(cmd) {
sendCalls.push(cmd);
const err = { $metadata: { httpStatusCode: 412 } };
throw err;
},
};

const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', {
'../../../src/storage/object/get.js': { default: mockGetObject },
'../../../src/storage/utils/version.js': {
ifMatch: () => mockIfMatchClient,
ifNoneMatch: () => mockIfMatchClient,
},
});

const resp = await putObjectWithVersion(
{ env: true },
{ users: [{ email: 'a@b.com' }] },
{ org: 'org', key: 'doc.html', type: 'text/html' },
null,
null,
{ ifMatch: '"client-specific-etag"' },
);

assert.equal(412, resp.status, 'Should propagate 412 when client sent a specific ETag');
assert.equal(1, sendCalls.length, 'Should not retry when client sent a specific ETag');
});

it('Put Object With Version store content', async () => {
// eslint-disable-next-line consistent-return
const mockGetObject = async (e, u, h) => {
Expand Down
Loading