Skip to content

Commit 2fea52d

Browse files
committed
fix(metadata-protocol): an OCC conflict re-reads the row as the caller; a row the caller cannot read answers not-found
Claude-Session: https://claude.ai/code/session_01YNquHRZKDpiKsgmHXSd9Jx Co-authored-by: Claude <noreply@anthropic.com>
1 parent ee0a5c0 commit 2fea52d

1 file changed

Lines changed: 51 additions & 11 deletions

File tree

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2857,7 +2857,9 @@ function simpleHash(str: string): string {
28572857
* The HTTP layer maps this to `409 Conflict` with code `CONCURRENT_UPDATE`,
28582858
* and includes both the current server-side version and the current record
28592859
* payload so the client can render an informed conflict-resolution UI
2860-
* ("Reload latest" vs. "Overwrite anyway").
2860+
* ("Reload latest" vs. "Overwrite anyway"). The record is the row as the
2861+
* caller reads it, and a row the caller cannot read is never a conflict: it
2862+
* answers not-found instead (#23586, see `assertVersionOf`).
28612863
*
28622864
* NOTE: This is an *application-level* compare-and-set — not an atomic
28632865
* storage-layer CAS. There is a small TOCTOU window between the version
@@ -14678,7 +14680,7 @@ export class ObjectStackProtocolImplementation implements
1467814680
// 404 wins over 409 when both could apply: OCC has always declined to
1467914681
// treat a missing record as a concurrency conflict, and "this record
1468014682
// does not exist" is the more specific answer.
14681-
this.assertVersionOf(request.object, request.id, current, request.expectedVersion);
14683+
await this.assertVersionOf(request.object, request.id, current, request.expectedVersion, request.context);
1468214684
const opts: any = { where: { id: request.id } };
1468314685
if (request.context !== undefined) opts.context = request.context;
1468414686
// [#3407/#3431] Capture the engine's LEGAL write strips (static `readonly`
@@ -14774,7 +14776,7 @@ export class ObjectStackProtocolImplementation implements
1477414776

1477514777
async deleteData(request: { object: string, id: string, expectedVersion?: string, context?: any }) {
1477614778
this.assertObjectRegistered(request.object); // [#3770]
14777-
await this.assertVersionMatch(request.object, request.id, request.expectedVersion);
14779+
await this.assertVersionMatch(request.object, request.id, request.expectedVersion, request.context);
1477814780
const opts: any = { where: { id: request.id } };
1477914781
if (request.context !== undefined) opts.context = request.context;
1478014782
const deleted = await this.engine.delete(request.object, opts);
@@ -14915,10 +14917,11 @@ export class ObjectStackProtocolImplementation implements
1491514917
}
1491614918

1491714919
/**
14918-
* Optimistic Concurrency Control — the COMPARISON half, over a row the
14919-
* caller has already read. Pure: it issues no query of its own, which is
14920-
* what lets `updateData` run the gate on its existence probe's result
14921-
* rather than re-reading the record (#4435).
14920+
* Optimistic Concurrency Control — the COMPARISON half, over the row the
14921+
* existence probe has already read. A token that agrees (or no token)
14922+
* issues no query of its own, which is what lets `updateData` run the gate
14923+
* on its existence probe's result rather than re-reading the record
14924+
* (#4435). Only a MISMATCH reads again — see the conflict paragraph below.
1492214925
*
1492314926
* When the caller passes a non-empty `expectedVersion` token (typically the
1492414927
* `updated_at` value they read), a mismatch throws `ConcurrentUpdateError`,
@@ -14948,13 +14951,35 @@ export class ObjectStackProtocolImplementation implements
1494814951
* SQLite family — must not decide whether a save is a conflict. See
1494914952
* `normaliseVersionToken` for the measured input domain and for why the
1495014953
* change cannot refuse a token that is accepted today.
14954+
*
14955+
* ## [#23586] The conflict reports only what the caller may read
14956+
*
14957+
* `current` is the SYSTEM-context probe ({@link probeRecord}: existence,
14958+
* not visibility), and this gate runs before `engine.update` /
14959+
* `engine.delete` apply the caller's policy. So a mismatch does not answer
14960+
* from it. It re-reads the row under the caller's own `context`, the read
14961+
* door's question (every RLS and FLS rule applies):
14962+
*
14963+
* - no row → `recordNotFoundError`, the producer the security layer's
14964+
* by-id write gate answers a row the caller cannot read with. The stale
14965+
* token then gets exactly the answer the same request without a token
14966+
* gets: no `currentVersion`, no `currentRecord`, so the `409` no longer
14967+
* tells a caller that a hidden row exists or when it was last written;
14968+
* - a row → the `409`, with THAT row as `currentRecord`, field-masked as
14969+
* the caller's own read is. `currentVersion` stays the token this
14970+
* comparison ran on.
14971+
*
14972+
* The read is issued only on a mismatch: a request whose token agrees, or
14973+
* that sends none, reads exactly what it read before. A read that throws
14974+
* (the caller's read grant withheld, a store fault) propagates as raised.
1495114975
*/
14952-
private assertVersionOf(
14976+
private async assertVersionOf(
1495314977
object: string,
1495414978
id: string,
1495514979
current: any,
1495614980
expectedVersion: string | undefined,
14957-
): void {
14981+
context: unknown,
14982+
): Promise<void> {
1495814983
assertVersionTokenNotMalformed(expectedVersion);
1495914984
const expected = normaliseVersionToken(expectedVersion);
1496014985
if (!expected) return;
@@ -14972,9 +14997,11 @@ export class ObjectStackProtocolImplementation implements
1497214997
// it names, turning a resolvable conflict into an unresolvable one.
1497314998
const currentVersion = currentToken.instant ?? currentToken.token;
1497414999
const expectedVersionText = expected.instant ?? expected.token;
15000+
const visible = await this.readAsCaller(object, id, context);
15001+
if (!visible) throw recordNotFoundError(object, id);
1497515002
throw new ConcurrentUpdateError({
1497615003
currentVersion,
14977-
currentRecord: current,
15004+
currentRecord: visible,
1497815005
message: `Record ${object}/${id} was modified by another user (current version ${currentVersion}, expected ${expectedVersionText})`,
1497915006
});
1498015007
}
@@ -15001,11 +15028,24 @@ export class ObjectStackProtocolImplementation implements
1500115028
object: string,
1500215029
id: string,
1500315030
expectedVersion: string | undefined,
15031+
context: unknown,
1500415032
): Promise<void> {
1500515033
assertVersionTokenNotMalformed(expectedVersion);
1500615034
if (!normaliseVersionToken(expectedVersion)) return;
1500715035
const current = await this.probeRecord(object, id);
15008-
this.assertVersionOf(object, id, current, expectedVersion);
15036+
await this.assertVersionOf(object, id, current, expectedVersion, context);
15037+
}
15038+
15039+
/**
15040+
* [#23586] The row as the CALLER reads it by id — the twin of
15041+
* {@link probeRecord}, which asks existence as the system. Same `context`
15042+
* spelling as the write it precedes: passed only when the request carried
15043+
* one, so a caller with no context reads as its write would run.
15044+
*/
15045+
private async readAsCaller(object: string, id: string, context: unknown): Promise<any> {
15046+
const opts: any = { where: { id } };
15047+
if (context !== undefined) opts.context = context;
15048+
return this.engine.findOne(object, opts);
1500915049
}
1501015050

1501115051
// ==========================================

0 commit comments

Comments
 (0)