From 0e080bfc5bc460d34b4ad252d8a82e33d010d1db Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Wed, 16 Sep 2026 21:18:38 -0400 Subject: [PATCH 01/14] Refuse a card write whose If-Match names a card that moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PATCH` and `DELETE` on a card+json URL now honour `If-Match`. A request carrying one is compared against the validator a `GET` of the card would hand out, and a card that no longer matches answers 412 with nothing staged, nothing enqueued and no event broadcast. `*` asks only that a card be there, which the handlers already settle, so it reaches their own 404 rather than a 412 that would say less. A card+json `GET`, `POST` and `PATCH` body also carries `meta.version` — the fingerprint of the `.json` the realm stores. The writes report the version their own commit minted; the read takes it off the `realm_file_meta` row the creation time already comes from, so it costs a column rather than a query. It is stripped at serialization, so a client echoing a served document back never persists one. Co-Authored-By: Claude Opus 5 (1M context) --- .../acceptance/code-submode/editor-test.ts | 6 +- .../host/tests/integration/realm-test.gts | 10 + .../tests/card-conditional-write-test.ts | 677 ++++++++++++++++++ .../realm-server/tests/card-endpoints-test.ts | 11 + .../tests/realm-endpoints-test.ts | 1 + .../card-operations/executors.ts | 8 +- packages/runtime-common/error.ts | 24 + packages/runtime-common/file-meta.ts | 57 ++ packages/runtime-common/file-serializer.ts | 4 + packages/runtime-common/realm.ts | 148 +++- packages/runtime-common/resource-types.ts | 13 + 11 files changed, 951 insertions(+), 8 deletions(-) create mode 100644 packages/realm-server/tests/card-conditional-write-test.ts diff --git a/packages/host/tests/acceptance/code-submode/editor-test.ts b/packages/host/tests/acceptance/code-submode/editor-test.ts index 0315bb6aef1..a0548cd3764 100644 --- a/packages/host/tests/acceptance/code-submode/editor-test.ts +++ b/packages/host/tests/acceptance/code-submode/editor-test.ts @@ -740,8 +740,12 @@ module('Acceptance | code submode | editor tests', function (hooks) { // `generation` is index metadata the realm stamps on the card+json GET; // it rides along in the loaded card the same way lastModified/realmInfo // do (and, like them, is stripped from the persisted source), so drop it - // before comparing against the card's source serialization. + // before comparing against the card's source serialization. `version` — + // the fingerprint of the stored file — rides and is stripped the same + // way, and describes the bytes this comparison is against rather than + // belonging to them. delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( stringify(json), stringify(expected), diff --git a/packages/host/tests/integration/realm-test.gts b/packages/host/tests/integration/realm-test.gts index 83ced3f4800..d98b2541624 100644 --- a/packages/host/tests/integration/realm-test.gts +++ b/packages/host/tests/integration/realm-test.gts @@ -759,6 +759,7 @@ module('Integration | realm', function (hooks) { let ownerCreatedAt = await getFileCreatedAt(realm, 'dir/owner.json'); let petCreatedAt = await getFileCreatedAt(realm, `Pet/${id}.json`); assert.ok(uuidValidate(id), 'card ID is a UUID'); + delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -1126,6 +1127,7 @@ module('Integration | realm', function (hooks) { ); assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -1303,6 +1305,7 @@ module('Integration | realm', function (hooks) { ); assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -1523,6 +1526,7 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -1647,6 +1651,7 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -1869,6 +1874,7 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -1994,6 +2000,7 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -2137,6 +2144,7 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -2272,6 +2280,7 @@ module('Integration | realm', function (hooks) { let json = await response.json(); let mangoCreatedAt = await getFileCreatedAt(realm, 'dir/mango.json'); let marikoCreatedAt = await getFileCreatedAt(realm, 'dir/mariko.json'); + delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -2462,6 +2471,7 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); + delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts new file mode 100644 index 00000000000..7efb68a8f05 --- /dev/null +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -0,0 +1,677 @@ +import QUnit from 'qunit'; +const { module, test } = QUnit; +import type { Test, SuperTest } from 'supertest'; +import { join, basename } from 'path'; +import type { RealmHttpServer as Server } from '../server.ts'; +import type { DirResult } from 'tmp'; +import fsExtra from 'fs-extra'; +const { existsSync, readFileSync } = fsExtra; +import type { Realm } from '@cardstack/runtime-common'; +import { computeContentHash, rri } from '@cardstack/runtime-common'; +import { + setupPermissionedRealmCached, + setupMatrixRoom, + closeServer, + withRealmPath, + type RealmRequest, +} from './helpers/index.ts'; +import { resetCatalogRealms } from '../handlers/handle-fetch-catalog-realms.ts'; +import type { PgAdapter } from '@cardstack/postgres'; +import type { MatrixEvent } from '@cardstack/base/matrix-event'; +import { APP_BOXEL_REALM_EVENT_TYPE } from '@cardstack/runtime-common/matrix-constants'; +import { waitForIncrementalIndexEvent } from './helpers/indexing.ts'; + +// The two conditional-write behaviors of the card+json facade, kept together +// because they are the two halves of one client story: `version` is what a +// client holds while it edits, and `If-Match` is how it refuses to overwrite a +// card that moved underneath it. +// +// They name different validators on purpose. `version` is the fingerprint of +// the `.json` the realm stores, so it moves only when that file is rewritten. +// The `ETag` describes the served *document*, so it also moves when a card +// this one links to is re-indexed — which is what makes it the HTTP cache +// validator, and what `If-Match` compares against. +module(basename(import.meta.filename), function () { + module('conditional card writes', function (hooks) { + let realmURL = new URL('http://127.0.0.1:4444/test/'); + let testRealm: Realm; + let testRealmHttpServer: Server; + let request: RealmRequest; + // The realm-scoped `request` above prefixes the realm's path, which is + // what the card URLs want and what `/_server-session` — a server route, + // not a realm one — must not have. + let serverRequest: SuperTest; + let dir: DirResult; + let dbAdapter: PgAdapter; + + function onRealmSetup(args: { + testRealm: Realm; + testRealmHttpServer: Server; + request: SuperTest; + dir: DirResult; + dbAdapter: PgAdapter; + }) { + testRealm = args.testRealm; + testRealmHttpServer = args.testRealmHttpServer; + request = withRealmPath(args.request, realmURL); + serverRequest = args.request; + dir = args.dir; + dbAdapter = args.dbAdapter; + } + + function getRealmSetup() { + return { + testRealm, + testRealmHttpServer, + request, + serverRequest, + dir, + dbAdapter, + }; + } + + function cardFile(name: string) { + return join(dir.name, 'realm_server_1', 'test', name); + } + + // The realm writes a card as `JSON.stringify(doc, null, 2)` and records + // the hash of exactly those bytes, so the file on disk is the whole input + // to the fingerprint a response reports. + function storedVersion(name: string) { + return computeContentHash(readFileSync(cardFile(name), 'utf8')); + } + + function patchPersonBody(firstName: string) { + return { + data: { + type: 'card', + attributes: { firstName }, + meta: { + adoptsFrom: { module: rri('./person.gts'), name: 'Person' }, + }, + }, + }; + } + + function patchFriendBody(firstName: string) { + return { + data: { + type: 'card', + attributes: { firstName }, + meta: { + adoptsFrom: { module: rri('./friend.gts'), name: 'Friend' }, + }, + }, + }; + } + + function incrementalIndexEvents(messages: MatrixEvent[]) { + return messages.filter( + (m) => + m.type === APP_BOXEL_REALM_EVENT_TYPE && + m.content.eventName === 'index' && + m.content.indexType === 'incremental', + ); + } + + hooks.afterEach(async function () { + await closeServer(testRealmHttpServer); + resetCatalogRealms(); + }); + + module('public writable realm', function (hooks) { + setupPermissionedRealmCached(hooks, { + fixture: 'realistic', + realmURL, + permissions: { + '*': ['read', 'write'], + '@node-test_realm:localhost': ['read', 'realm-owner'], + }, + onRealmSetup, + }); + + let { getMessagesSince } = setupMatrixRoom(hooks, getRealmSetup); + + test('a PATCH naming a validator the card has moved past is refused, and writes nothing', async function (assert) { + let firstRead = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let staleEtag = firstRead.get('etag') ?? ''; + assert.ok(staleEtag, 'the read hands out a validator'); + + let moved = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + moved.status, + 200, + `the unconditional write succeeds: ${moved.text}`, + ); + assert.notStrictEqual( + moved.get('etag'), + staleEtag, + 'and moves the card past the validator read above', + ); + let bytesBefore = readFileSync(cardFile('person-1.json'), 'utf8'); + + let response = await request + .patch('/person-1') + .send(patchPersonBody('Paper')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', staleEtag); + + assert.strictEqual(response.status, 412, 'HTTP 412 status'); + assert.strictEqual( + readFileSync(cardFile('person-1.json'), 'utf8'), + bytesBefore, + 'the stored file is untouched', + ); + let after = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + after.body?.data?.attributes?.firstName, + 'Van Gogh', + 'and the card still reads as the write before it left it', + ); + }); + + test('a PATCH naming the validator the card still carries is applied', async function (assert) { + let read = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let etag = read.get('etag') ?? ''; + assert.ok(etag, 'the read hands out a validator'); + + let response = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', etag); + + assert.strictEqual( + response.status, + 200, + `HTTP 200 status: ${response.text}`, + ); + assert.strictEqual( + response.body?.data?.attributes?.firstName, + 'Van Gogh', + 'and answers with the patched card, as an unconditional PATCH does', + ); + }); + + test('a PATCH asking only that the card be there is applied', async function (assert) { + let response = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', '*'); + + assert.strictEqual( + response.status, + 200, + `HTTP 200 status: ${response.text}`, + ); + assert.strictEqual( + response.body?.data?.attributes?.firstName, + 'Van Gogh', + 'and the patch lands', + ); + }); + + test('a validator the realm never issued is refused whatever its spelling', async function (assert) { + for (let ifMatch of [ + '"not-a-validator"', + 'W/"not-a-validator"', + '"one", "two"', + ]) { + let response = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', ifMatch); + assert.strictEqual( + response.status, + 412, + `If-Match: ${ifMatch} is refused`, + ); + } + }); + + test('a validator the realm issued matches when the client echoes it as weak', async function (assert) { + let read = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let etag = read.get('etag') ?? ''; + assert.ok(etag, 'the read hands out a validator'); + + let response = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', `W/${etag}`); + + assert.strictEqual( + response.status, + 200, + `the W/ prefix is ignored on both sides: ${response.text}`, + ); + }); + + test('a refused write enqueues no indexing and broadcasts no event', async function (assert) { + let read = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let staleEtag = read.get('etag') ?? ''; + let moved = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(moved.status, 200, 'the card is moved past it'); + + let since = Date.now(); + let refused = await request + .patch('/person-1') + .send(patchPersonBody('Paper')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', staleEtag); + assert.strictEqual( + refused.status, + 412, + 'the conditional write is refused', + ); + + // The control: a write the realm does accept, made after the refused + // one and in the same window. Its event is what proves the window is + // one an event can arrive in — without it, "no event" would also be + // satisfied by a listener that sees nothing at all. The refused write + // came first, so anything it broadcast is already visible by the time + // this one's event is. + let accepted = await request + .patch('/person-1') + .send(patchPersonBody('Paper')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(accepted.status, 200, 'the control write succeeds'); + await waitForIncrementalIndexEvent(getMessagesSince, since); + + let events = incrementalIndexEvents(await getMessagesSince(since)); + assert.strictEqual( + events.length, + 1, + 'exactly one write in the window reached the index', + ); + assert.deepEqual( + (events[0].content as { invalidations?: string[] }).invalidations, + [`${realmURL.href}person-1`], + 'and it is the one the realm accepted', + ); + }); + + test('a DELETE naming a validator the card has moved past is refused, and removes nothing', async function (assert) { + let read = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let staleEtag = read.get('etag') ?? ''; + let moved = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(moved.status, 200, 'the card is moved past it'); + + let response = await request + .delete('/person-1') + .set('Accept', 'application/vnd.card+json') + .set('If-Match', staleEtag); + + assert.strictEqual(response.status, 412, 'HTTP 412 status'); + assert.true( + existsSync(cardFile('person-1.json')), + 'the card is still on disk', + ); + let after = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(after.status, 200, 'and still serves'); + }); + + test('a DELETE naming the validator the card still carries removes it', async function (assert) { + let read = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let etag = read.get('etag') ?? ''; + assert.ok(etag, 'the read hands out a validator'); + + let response = await request + .delete('/person-1') + .set('Accept', 'application/vnd.card+json') + .set('If-Match', etag); + + assert.strictEqual( + response.status, + 204, + `HTTP 204 status: ${response.text}`, + ); + assert.false( + existsSync(cardFile('person-1.json')), + 'and the card is gone', + ); + }); + + test('a DELETE asking only that the card be there removes it', async function (assert) { + let response = await request + .delete('/person-1') + .set('Accept', 'application/vnd.card+json') + .set('If-Match', '*'); + + assert.strictEqual( + response.status, + 204, + `HTTP 204 status: ${response.text}`, + ); + assert.false( + existsSync(cardFile('person-1.json')), + 'and the card is gone', + ); + }); + + test('asking only that a card be there says nothing about one that is not', async function (assert) { + // `*` is a question about existence, which these handlers answer for + // themselves — so it reaches the 404 an unconditional request would, + // rather than a 412 that would say less. + let unconditional = await request + .delete('/no-such-card') + .set('Accept', 'application/vnd.card+json'); + let conditional = await request + .delete('/no-such-card') + .set('Accept', 'application/vnd.card+json') + .set('If-Match', '*'); + + assert.strictEqual( + conditional.status, + unconditional.status, + 'the two answer alike', + ); + assert.strictEqual(conditional.status, 404, 'with a 404'); + }); + + test('a body the realm would refuse anyway is still answered by what is wrong with it', async function (assert) { + let response = await request + .patch('/person-1') + .send({ data: 'not a card resource' }) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', '"not-a-validator"'); + + assert.strictEqual( + response.status, + 400, + 'the payload is reported before the precondition it would also fail', + ); + }); + + test('a write carrying no If-Match is unaffected by any of this', async function (assert) { + let response = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + + assert.strictEqual( + response.status, + 200, + `HTTP 200 status: ${response.text}`, + ); + let removal = await request + .delete('/person-1') + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(removal.status, 204, 'and so is the removal'); + }); + + test('a PATCH reports the version of the file it wrote, and it moves with the bytes', async function (assert) { + let first = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(first.status, 200, `HTTP 200 status: ${first.text}`); + assert.strictEqual( + first.body?.data?.meta?.version, + storedVersion('person-1.json'), + 'the version is the fingerprint of the stored file', + ); + + let second = await request + .patch('/person-1') + .send(patchPersonBody('Paper')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + second.status, + 200, + `HTTP 200 status: ${second.text}`, + ); + assert.strictEqual( + second.body?.data?.meta?.version, + storedVersion('person-1.json'), + 'and again after the second write', + ); + assert.notStrictEqual( + second.body?.data?.meta?.version, + first.body?.data?.meta?.version, + 'a write that changes the card changes its version', + ); + }); + + test('a PATCH that changes nothing reports the version the card already held', async function (assert) { + let write = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(write.status, 200, `HTTP 200 status: ${write.text}`); + let version = write.body?.data?.meta?.version; + assert.ok(version, 'the write reports a version'); + + let noop = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + + assert.strictEqual(noop.status, 200, `HTTP 200 status: ${noop.text}`); + assert.strictEqual( + noop.body?.data?.meta?.version, + version, + 'a patch that leaves the file alone leaves the version alone', + ); + assert.strictEqual( + noop.body?.data?.meta?.version, + storedVersion('person-1.json'), + 'and it is still the fingerprint of what is stored', + ); + }); + + test('a POST reports the version of the file it created', async function (assert) { + let response = await request + .post('/') + .send({ + data: { + type: 'card', + attributes: { firstName: 'Mango' }, + meta: { + adoptsFrom: { module: rri('./person.gts'), name: 'Person' }, + }, + }, + }) + .set('Accept', 'application/vnd.card+json'); + + assert.strictEqual( + response.status, + 201, + `HTTP 201 status: ${response.text}`, + ); + let id: string = response.body?.data?.id; + assert.ok(id, 'the create reports the id it minted'); + assert.strictEqual( + response.body?.data?.meta?.version, + storedVersion(`Person/${id.split('/').pop()}.json`), + 'the version is the fingerprint of the file it wrote', + ); + }); + + test('a GET reports the version the realm last wrote for the card', async function (assert) { + // Only the realm's own write path records a content hash, so a card + // that reached disk another way — as this realm's cards did — carries + // no version until it is next written. + let beforeAnyWrite = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(beforeAnyWrite.status, 200, 'the card serves'); + assert.strictEqual( + beforeAnyWrite.body?.data?.meta?.version, + undefined, + 'and reports no version for a file the realm has never written', + ); + + let write = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(write.status, 200, `HTTP 200 status: ${write.text}`); + + let response = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + response.body?.data?.meta?.version, + storedVersion('person-1.json'), + 'the read reports the fingerprint of the stored file', + ); + assert.strictEqual( + response.body?.data?.meta?.version, + write.body?.data?.meta?.version, + 'which is the version the write that produced it reported', + ); + }); + + test("a linked card's change moves the validator and leaves the version alone", async function (assert) { + // The whole reason there are two: `hassan` links to `jade`, so writing + // `jade` re-indexes `hassan` and changes what a GET of it serves — but + // `hassan.json` is not rewritten, so the base an optimistic client + // holds for it must not move. + let write = await request + .patch('/hassan') + .send(patchFriendBody('Hassan')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(write.status, 200, `HTTP 200 status: ${write.text}`); + + let before = await request + .get('/hassan') + .set('Accept', 'application/vnd.card+json'); + let etagBefore = before.get('etag') ?? ''; + let versionBefore = before.body?.data?.meta?.version; + assert.ok(etagBefore, 'the card carries a validator'); + assert.ok(versionBefore, 'and a version'); + + let linked = await request + .patch('/jade') + .send(patchFriendBody('Jade Vincent')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + linked.status, + 200, + `the linked card is written: ${linked.text}`, + ); + + let after = await request + .get('/hassan') + .set('Accept', 'application/vnd.card+json'); + assert.notStrictEqual( + after.get('etag'), + etagBefore, + 'the re-index moves the cache validator', + ); + assert.strictEqual( + after.body?.data?.meta?.version, + versionBefore, + 'and leaves the version, which describes only this card’s own file', + ); + }); + + test('a client echoing a version back does not persist it', async function (assert) { + let response = await request + .patch('/person-1') + .send({ + data: { + type: 'card', + attributes: { firstName: 'Van Gogh' }, + meta: { + adoptsFrom: { module: rri('./person.gts'), name: 'Person' }, + version: 'a-version-the-client-was-served', + }, + }, + }) + .set('Accept', 'application/vnd.card+json'); + + assert.strictEqual( + response.status, + 200, + `HTTP 200 status: ${response.text}`, + ); + let stored = JSON.parse( + readFileSync(cardFile('person-1.json'), 'utf8'), + ); + assert.strictEqual( + stored.data.meta.version, + undefined, + 'the stored file names no version', + ); + assert.strictEqual( + response.body?.data?.meta?.version, + storedVersion('person-1.json'), + 'and the response reports the one the realm computed', + ); + }); + }); + + // A card read picks its link shape per request, and the shapes take + // different variants of one validator so a client holding either is never + // 304'd to the other. That distinction is about representations; a + // conditional write asks about the card, so a client that read it under + // the narrower shape must not be refused for it. + module('a realm serving the narrower read shape', function (hooks) { + setupPermissionedRealmCached(hooks, { + fixture: 'realistic', + realmURL, + permissions: { + '*': ['read', 'write'], + '@node-test_realm:localhost': ['read', 'realm-owner'], + }, + liveReadsResolveLinksOnly: true, + onRealmSetup, + }); + + test('a validator issued by a links-only read still names the card', async function (assert) { + let read = await request + .get('/hassan') + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(read.status, 200, `HTTP 200 status: ${read.text}`); + assert.strictEqual( + (read.body?.included ?? []).length, + 0, + 'the read answered the link without side-loading its target', + ); + let etag = read.get('etag') ?? ''; + assert.ok(etag, 'and handed out a validator'); + + let response = await request + .patch('/hassan') + .send(patchFriendBody('Hassan')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', etag); + + assert.strictEqual( + response.status, + 200, + `the write is not refused over the shape it was read in: ${response.text}`, + ); + }); + }); + }); +}); diff --git a/packages/realm-server/tests/card-endpoints-test.ts b/packages/realm-server/tests/card-endpoints-test.ts index 1fe15eac391..5ee7cf2653e 100644 --- a/packages/realm-server/tests/card-endpoints-test.ts +++ b/packages/realm-server/tests/card-endpoints-test.ts @@ -2567,6 +2567,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -2702,6 +2703,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -2809,6 +2811,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -2850,6 +2853,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -3061,6 +3065,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4066,6 +4071,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4201,6 +4207,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4308,6 +4315,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4349,6 +4357,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4535,6 +4544,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4620,6 +4630,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, diff --git a/packages/realm-server/tests/realm-endpoints-test.ts b/packages/realm-server/tests/realm-endpoints-test.ts index 606bd4db6ec..00f6c7a4b5b 100644 --- a/packages/realm-server/tests/realm-endpoints-test.ts +++ b/packages/realm-server/tests/realm-endpoints-test.ts @@ -954,6 +954,7 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; + delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, diff --git a/packages/runtime-common/card-operations/executors.ts b/packages/runtime-common/card-operations/executors.ts index fb22c268702..0737c7177ab 100644 --- a/packages/runtime-common/card-operations/executors.ts +++ b/packages/runtime-common/card-operations/executors.ts @@ -706,13 +706,15 @@ export async function stageUpdate( // // Realm-managed keys never come from a patch: `realmInfo` and `realmURL` are // stamped by the realm serving the card, `screenshots` is joined from the - // prerendered manifest at serve time, and `type` is fixed by the document - // shape. A client echoing back what it was served must not persist any of - // them into the source file. + // prerendered manifest at serve time, `version` is the fingerprint of the + // bytes the realm last stored, and `type` is fixed by the document shape. A + // client echoing back what it was served must not persist any of them into + // the source file. delete (patch as { type?: unknown }).type; delete patch.meta.realmInfo; delete patch.meta.realmURL; delete patch.meta.screenshots; + delete patch.meta.version; promoteStagedLinks(patch, ctx); diff --git a/packages/runtime-common/error.ts b/packages/runtime-common/error.ts index c80421e69bb..a1fa2bd8cf3 100644 --- a/packages/runtime-common/error.ts +++ b/packages/runtime-common/error.ts @@ -940,6 +940,30 @@ export function notAcceptable( ); } +// The card at the URL is not the one the request's `If-Match` names, so the +// write is refused before anything is staged. The status is the whole answer a +// conditional client acts on — it re-reads the card and decides what to do +// about the edit it was holding — so the body only names which validator was +// asked for. +export function preconditionFailed({ + request, + requestContext, + id, +}: { + request: Request; + requestContext: RequestContext; + id: string; +}): Response { + return responseWithError( + new CardError( + `${request.method} of ${id} requires the card to match ` + + `If-Match: ${request.headers.get('if-match')}, and it does not`, + { status: 412, id }, + ), + requestContext, + ); +} + export function badRequest({ message, requestContext, diff --git a/packages/runtime-common/file-meta.ts b/packages/runtime-common/file-meta.ts index 0adbedfa750..fe9522ec8ec 100644 --- a/packages/runtime-common/file-meta.ts +++ b/packages/runtime-common/file-meta.ts @@ -53,6 +53,63 @@ export async function getContentMeta( }; } +// Everything `realm_file_meta` records about one file, in one row read. +// +// The two single-path readers above answer one column group each, so a caller +// that wants both pays two round-trips for one row. A card+json GET is exactly +// that caller: it reports the file's creation time as `x-created` and its +// content hash as the card's `meta.version`. +export async function getFileMeta( + db: DBAdapter, + realmURL: string, + localPath: string, +): Promise<{ + createdAt: number | undefined; + contentHash: string | undefined; + contentSize: number | undefined; +}> { + let absent = { + createdAt: undefined, + contentHash: undefined, + contentSize: undefined, + }; + if (!db) { + return absent; + } + let rows = await query(db, [ + 'SELECT created_at, content_hash, content_size FROM realm_file_meta WHERE realm_url =', + param(realmURL), + 'AND file_path =', + param(localPath), + 'LIMIT 1', + ]); + if (!rows || rows.length === 0) { + return absent; + } + let createdAt = rows[0]['created_at']; + let contentHash = rows[0]['content_hash']; + let contentSize = rows[0]['content_size']; + return { + // Each column is absent on its own: indexing inserts `created_at` alone + // for a file that reached disk outside the realm's write API, and only + // the write path ever fills the hash columns, so a row can hold either + // without the other. + createdAt: + createdAt == null + ? undefined + : typeof createdAt === 'string' + ? parseInt(createdAt) + : Number(createdAt), + contentHash: contentHash == null ? undefined : String(contentHash), + contentSize: + contentSize == null + ? undefined + : typeof contentSize === 'string' + ? parseInt(contentSize) + : Number(contentSize), + }; +} + // Reads created_at + content hash/size for many paths in a single query. Only // paths with a persisted row are returned; a caller treats an absent path as // "no persisted meta for this file" and falls back to its own per-path read. diff --git a/packages/runtime-common/file-serializer.ts b/packages/runtime-common/file-serializer.ts index cc59dd6bfdb..8875d9dca99 100644 --- a/packages/runtime-common/file-serializer.ts +++ b/packages/runtime-common/file-serializer.ts @@ -122,6 +122,10 @@ export default async function serialize({ // the doc was GET from) — persisting an echo would pin a copied card's // source file to the original's captures. delete result.data.meta.screenshots; + // The fingerprint of these very bytes, reported alongside them when the card + // is served. Writing it into the file it describes would make the file name + // a version it no longer holds the moment it lands. + delete result.data.meta.version; delete result.included; delete result.data.links; result.data.type = 'card'; diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index d8e4b49ea10..3a1ff5042b5 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -87,6 +87,7 @@ import { removeFileMeta, getCreatedTime, getContentMeta, + getFileMeta, getFileMetaForPaths, } from './file-meta.ts'; import { @@ -96,6 +97,7 @@ import { notAcceptable, methodNotAllowed, badRequest, + preconditionFailed, CardError, responseWithError, formattedError, @@ -7737,6 +7739,11 @@ export class Realm { }, }); } + // The version the commit just minted, reported by the write that produced + // it rather than read back out of `realm_file_meta` — a second read would + // answer for whatever the file holds now, which after a concurrent write + // is not what this response carries. + doc.data.meta.version = result.meta.version; this.#serveInstanceIdsAsRRI(doc); return createResponse({ body: JSON.stringify(doc, null, 2), @@ -7752,6 +7759,69 @@ export class Realm { }); } + // A conditional write: the 412 a `PATCH` or `DELETE` carrying `If-Match` + // answers when the card is no longer the one the caller saw, or `undefined` + // for a request that may proceed. Called before the commit, so a refused + // write stages nothing, enqueues no indexing and broadcasts no event. + // + // The comparand is the card's `ETag` — the same validator a `GET` of it + // hands out and an `If-None-Match` is compared against — not the card's + // `version`. That is what an HTTP conditional request names, and it is the + // stricter of the two: it moves when the served document may differ, so a + // client that revalidated its copy and then wrote holds a validator that + // still describes what it read. + // + // Comparison is `ifNoneMatchMatches`': `*`, comma lists, and the `W/` prefix + // ignored on both sides. RFC 9110 §13.1.1 asks for strong comparison here + // where §13.1.2 allows weak, but the realm emits no weak validators, so the + // two rules differ only over a validator no response of ours produced. + // + // `*` asks only that a card be there, which is what the handler settles + // itself — an absent one gets the 404 it always would, which says more than + // a 412 does. A concrete validator has to match one, so a card the realm + // can offer no validator for — never indexed, or an `ETag` suppressed + // because the document depends on another realm — fails the precondition: + // "this is the card you saw" is a claim the realm cannot make. + async #conditionalWriteRefusal( + request: Request, + url: URL, + requestContext: RequestContext, + ): Promise { + let ifMatch = request.headers.get('if-match'); + if (!ifMatch) { + return undefined; + } + if (ifMatch.trim() === '*') { + return undefined; + } + await this.getRealmInfo(); + let entry = await this.#realmIndexQueryEngine.instance(url, { + includeErrors: true, + }); + if (entry?.type !== 'instance' || this.hasForeignRealmDeps(entry.deps)) { + return preconditionFailed({ request, requestContext, id: url.href }); + } + // Every validator the realm would hand out for this card as it stands. + // A card+json read picks its link shape per request — the setting can + // differ between the read that gave the client its validator and this + // write — and the shapes take different variants of one validator so a + // client holding either is not 304'd to the other. That distinction is + // about representations; this question is about the card, and all of + // these describe the same card at the same `indexed_at`. Refusing a write + // because the read that preceded it answered in the narrower shape would + // refuse on a server setting rather than on anything the caller did. + let realmInfoHash = this.getCachedRealmInfoHash(); + let screenshots = screenshotsEtagFingerprint(entry.screenshots); + let issued = [ + buildCardJsonEtag(entry.indexedAt, realmInfoHash, screenshots, false), + buildCardJsonEtag(entry.indexedAt, realmInfoHash, screenshots, true), + ]; + if (issued.some((etag) => etag && ifNoneMatchMatches(ifMatch, etag))) { + return undefined; + } + return preconditionFailed({ request, requestContext, id: url.href }); + } + private async patchCardInstance( request: Request, requestContext: RequestContext, @@ -7799,6 +7869,18 @@ export class Realm { } } + // Read after the body, so a payload the realm would have refused anyway + // still gets the 400 that names what is wrong with it, and a 412 means + // only that the card moved. + let refusal = await this.#conditionalWriteRefusal( + request, + new URL(instanceURL), + requestContext, + ); + if (refusal) { + return refusal; + } + // The merge belongs to the update this stages: arrays replacing rather // than merging into what is there, the realm-managed `meta` keys a client // echo must never persist, a type that cannot change, the relationship @@ -7865,6 +7947,7 @@ export class Realm { }); } let created = result.meta.created; + let version = result.meta.version; let readEntry = async (skipQueryBackedExpansion: boolean) => await this.#realmIndexQueryEngine.cardDocument(new URL(instanceURL), { loadLinks: true, @@ -7886,6 +7969,10 @@ export class Realm { // the one the index recorded for it. lastModified: unchanged.doc.data.meta.lastModified ?? lastModified, created, + // Nothing was written, so this is the version the card already held + // — which is the point of reporting it: a patch that changes nothing + // leaves a client's base where it was. + version, requestContext, }); } @@ -7904,6 +7991,7 @@ export class Realm { } lastModified = result?.meta.lastModified ?? lastModified; created = result?.meta.created ?? created; + version = result?.meta.version ?? version; } if (answerFromEcho) { // See serializedInstanceEcho: the write indexed deferred, so there is @@ -7923,6 +8011,7 @@ export class Realm { instanceURL, lastModified, ); + doc.data.meta.version = version; this.#serveInstanceIdsAsRRI(doc); return createResponse({ body: JSON.stringify(doc, null, 2), @@ -7944,6 +8033,7 @@ export class Realm { localPath, lastModified, created, + version, requestContext, }); } @@ -7969,6 +8059,7 @@ export class Realm { meta: { ...(stored.data.meta ?? {}), lastModified, + version, }, }, }) as SingleCardDocument; @@ -8005,19 +8096,26 @@ export class Realm { localPath, lastModified, created, + version, requestContext, }: { instanceURL: string; localPath: LocalPath; lastModified: number | null; created: number | null; + version: string; requestContext: RequestContext; }, ): Promise { let doc: SingleCardDocument = merge({}, entry.doc, { data: { links: { self: instanceURL }, - meta: { lastModified }, + // The version the commit reported for the file it left behind, which + // is the one this response's bytes describe. The index row this + // document came from carries no fingerprint of the source, and reading + // one back would answer for the file as it stands now rather than as + // this write left it. + meta: { lastModified, version }, }, }); // The PATCH echo carries the joined `meta.screenshots` like a GET does — @@ -8735,7 +8833,7 @@ export class Realm { ); } let { document, headers, queryBacked } = result; - if (document.data.type === 'file-meta') { + if (!isSingleCardDocument(document)) { // A file's metadata is derived from its bytes, so there is no index row // to validate it against and nothing here to cache. return { kind: 'file-meta', body: JSON.stringify(document, null, 2) }; @@ -8751,9 +8849,31 @@ export class Realm { return { kind: 'redirect', foundPath }; } - // Prefer created_at from DB for instance JSON + // Prefer created_at from DB for instance JSON. The card's `version` comes + // out of the same row, so reporting it costs this read's column list + // rather than a second round-trip. let pathForDb = this.paths.local(url) + '.json'; - let createdAt = await this.getCreatedTime(pathForDb); + let { createdAt, contentHash } = await this.storedFileMetaFor(pathForDb); + // The card's write identity: the fingerprint of the `.json` the realm + // stores, which moves only when that file is rewritten. Deliberately not + // the `ETag` beside it — that one is the HTTP cache validator and tracks + // the served *document*, so it also moves when a card this one links to is + // re-indexed. A client holding an optimistic edit reconciles on `version`. + // + // Only the realm's own write path records a hash, so a card that reached + // disk some other way — a deployed or seeded realm, a fixture copied into + // place — has none until it is next written, and the key is absent rather + // than computed: hashing the file to answer a read would put a bounded but + // real file read on every uncached GET of every such card. + // + // Read after the document and reported with it, so the pair a caller acts + // on is the pair one assembly produced. The response cache keeps them + // together for the same reason: it retains the serialized body, so a + // cached document and the version it was assembled beside are served + // together or not at all. + if (contentHash !== undefined) { + card.data.meta.version = contentHash; + } // deps + indexedAt come off the assembly the read reports, not off the // early peek: the two see different snapshots when a write lands between // them, and a validator has to describe the bytes it is sent with. @@ -9080,6 +9200,14 @@ export class Realm { if (await this.openFileForMetadata(localPath)) { return methodNotAllowed(request, requestContext); } + let refusal = await this.#conditionalWriteRefusal( + request, + url, + requestContext, + ); + if (refusal) { + return refusal; + } try { // Whether there is a card here is settled by the stored file, read // inside the same lock the removal happens under. That is what makes a @@ -9109,6 +9237,18 @@ export class Realm { return getCreatedTime(this.#dbAdapter, this.url, path); } + // The whole `realm_file_meta` row for a path, for a caller that reports more + // than one of its columns and would otherwise read the row once per column. + private async storedFileMetaFor(path: LocalPath): Promise<{ + createdAt: number | undefined; + contentHash: string | undefined; + }> { + if (!this.#dbAdapter) { + return { createdAt: undefined, contentHash: undefined }; + } + return getFileMeta(this.#dbAdapter, this.url, path); + } + private async directoryEntries( url: URL, ): Promise<{ name: string; kind: Kind; path: LocalPath }[] | undefined> { diff --git a/packages/runtime-common/resource-types.ts b/packages/runtime-common/resource-types.ts index f29bbbd849b..b672ea848d4 100644 --- a/packages/runtime-common/resource-types.ts +++ b/packages/runtime-common/resource-types.ts @@ -119,6 +119,19 @@ export type CardResourceMeta = Meta & { // writes like `realmInfo`/`realmURL`. The `screenshotURLs` getter on // CardDef/FileDef reads this. screenshots?: ScreenshotsMeta; + // The fingerprint of the `.json` file the realm stores for this card — the + // card's write identity, and the base an optimistic client reconciles + // against. It moves only when that file is rewritten, so it is stable across + // the re-indexes a linked card's change causes. + // + // Distinct from the HTTP `ETag` on the same response, which tracks the + // served document rather than the stored file and is therefore the wrong + // thing to hold an edit against — though it is what a conditional write + // (`If-Match`) names, because that is a cache validator's job. + // + // Serve-time output like the keys above: stripped from a write's payload, so + // a client echoing back what it was served never persists one. + version?: string; }; export type FileMetaResourceResourceMeta = Meta & { From 68a3852fbdfd86991570389f6170a83f891e2aca Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Wed, 16 Sep 2026 21:50:19 -0400 Subject: [PATCH 02/14] Create the version fixture from a base type, not a realm-relative one A create resolves its module reference against the directory it mints the card into, one level below where the `POST` was aimed, so a realm-relative reference names a module that is not there. Co-Authored-By: Claude Opus 5 (1M context) --- .../realm-server/tests/card-conditional-write-test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts index 7efb68a8f05..580b8c48ba6 100644 --- a/packages/realm-server/tests/card-conditional-write-test.ts +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -493,9 +493,12 @@ module(basename(import.meta.filename), function () { .send({ data: { type: 'card', - attributes: { firstName: 'Mango' }, + attributes: { cardInfo: { name: 'Mango' } }, meta: { - adoptsFrom: { module: rri('./person.gts'), name: 'Person' }, + adoptsFrom: { + module: rri('@cardstack/base/card-api'), + name: 'CardDef', + }, }, }, }) @@ -510,7 +513,7 @@ module(basename(import.meta.filename), function () { assert.ok(id, 'the create reports the id it minted'); assert.strictEqual( response.body?.data?.meta?.version, - storedVersion(`Person/${id.split('/').pop()}.json`), + storedVersion(`${id.slice(realmURL.href.length)}.json`), 'the version is the fingerprint of the file it wrote', ); }); From 782ba8bebd4aa555fe3371e43076f3f73b6a8528 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Wed, 16 Sep 2026 22:28:49 -0400 Subject: [PATCH 03/14] Evaluate the precondition against a drained index, and scope to If-Match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A card's `ETag` is built from `indexed_at`, which moves after the bytes rather than with them: a commit records the file's hash before it indexes, and a deferred write indexes on a worker. Read without draining, the precondition consults a row that still spells a validator the caller has already been overtaken by, and accepts the write it exists to refuse — with no concurrency in the request at all. It drains first, except for the callers that cannot wait on indexing without deadlocking. `meta.version` is no longer carried on card+json responses. Nothing reads it, and adding a key to the served read representation costs every cached card body a revalidation. It belongs with the event and envelope surfaces that will consume it, and is specified there. Co-Authored-By: Claude Opus 5 (1M context) --- .../acceptance/code-submode/editor-test.ts | 6 +- .../host/tests/integration/realm-test.gts | 10 - .../tests/card-conditional-write-test.ts | 292 +++--------------- .../realm-server/tests/card-endpoints-test.ts | 11 - .../tests/realm-endpoints-test.ts | 1 - .../card-operations/executors.ts | 8 +- packages/runtime-common/file-meta.ts | 57 ---- packages/runtime-common/file-serializer.ts | 4 - packages/runtime-common/realm.ts | 106 +++---- packages/runtime-common/resource-types.ts | 13 - 10 files changed, 95 insertions(+), 413 deletions(-) diff --git a/packages/host/tests/acceptance/code-submode/editor-test.ts b/packages/host/tests/acceptance/code-submode/editor-test.ts index a0548cd3764..0315bb6aef1 100644 --- a/packages/host/tests/acceptance/code-submode/editor-test.ts +++ b/packages/host/tests/acceptance/code-submode/editor-test.ts @@ -740,12 +740,8 @@ module('Acceptance | code submode | editor tests', function (hooks) { // `generation` is index metadata the realm stamps on the card+json GET; // it rides along in the loaded card the same way lastModified/realmInfo // do (and, like them, is stripped from the persisted source), so drop it - // before comparing against the card's source serialization. `version` — - // the fingerprint of the stored file — rides and is stripped the same - // way, and describes the bytes this comparison is against rather than - // belonging to them. + // before comparing against the card's source serialization. delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( stringify(json), stringify(expected), diff --git a/packages/host/tests/integration/realm-test.gts b/packages/host/tests/integration/realm-test.gts index d98b2541624..83ced3f4800 100644 --- a/packages/host/tests/integration/realm-test.gts +++ b/packages/host/tests/integration/realm-test.gts @@ -759,7 +759,6 @@ module('Integration | realm', function (hooks) { let ownerCreatedAt = await getFileCreatedAt(realm, 'dir/owner.json'); let petCreatedAt = await getFileCreatedAt(realm, `Pet/${id}.json`); assert.ok(uuidValidate(id), 'card ID is a UUID'); - delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -1127,7 +1126,6 @@ module('Integration | realm', function (hooks) { ); assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -1305,7 +1303,6 @@ module('Integration | realm', function (hooks) { ); assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -1526,7 +1523,6 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -1651,7 +1647,6 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -1874,7 +1869,6 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -2000,7 +1994,6 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -2144,7 +2137,6 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json.data, { type: 'card', id: `${testRealmURL}jackie`, @@ -2280,7 +2272,6 @@ module('Integration | realm', function (hooks) { let json = await response.json(); let mangoCreatedAt = await getFileCreatedAt(realm, 'dir/mango.json'); let marikoCreatedAt = await getFileCreatedAt(realm, 'dir/mariko.json'); - delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', @@ -2471,7 +2462,6 @@ module('Integration | realm', function (hooks) { assert.strictEqual(response.status, 200, 'successful http status'); let json = await response.json(); - delete json.data.meta.version; assert.deepEqual(json, { data: { type: 'card', diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts index 580b8c48ba6..07fed1b1355 100644 --- a/packages/realm-server/tests/card-conditional-write-test.ts +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -7,7 +7,7 @@ import type { DirResult } from 'tmp'; import fsExtra from 'fs-extra'; const { existsSync, readFileSync } = fsExtra; import type { Realm } from '@cardstack/runtime-common'; -import { computeContentHash, rri } from '@cardstack/runtime-common'; +import { rri } from '@cardstack/runtime-common'; import { setupPermissionedRealmCached, setupMatrixRoom, @@ -21,16 +21,15 @@ import type { MatrixEvent } from '@cardstack/base/matrix-event'; import { APP_BOXEL_REALM_EVENT_TYPE } from '@cardstack/runtime-common/matrix-constants'; import { waitForIncrementalIndexEvent } from './helpers/indexing.ts'; -// The two conditional-write behaviors of the card+json facade, kept together -// because they are the two halves of one client story: `version` is what a -// client holds while it edits, and `If-Match` is how it refuses to overwrite a -// card that moved underneath it. +// A conditional write: `If-Match` on a card+json `PATCH` or `DELETE`, which +// lets a client say "only apply this if the card is still the one I saw" and +// be refused rather than silently overwrite a card that moved underneath it. // -// They name different validators on purpose. `version` is the fingerprint of -// the `.json` the realm stores, so it moves only when that file is rewritten. -// The `ETag` describes the served *document*, so it also moves when a card -// this one links to is re-indexed — which is what makes it the HTTP cache -// validator, and what `If-Match` compares against. +// The validator it names is the card's `ETag` — the one a `GET` of the card +// hands out, built from the index row. That is what an HTTP conditional +// request names, and the realm's read-your-own-writes contract is what makes +// it usable: a client is served its own writes, so the validator it holds +// describes what it last read. module(basename(import.meta.filename), function () { module('conditional card writes', function (hooks) { let realmURL = new URL('http://127.0.0.1:4444/test/'); @@ -74,13 +73,6 @@ module(basename(import.meta.filename), function () { return join(dir.name, 'realm_server_1', 'test', name); } - // The realm writes a card as `JSON.stringify(doc, null, 2)` and records - // the hash of exactly those bytes, so the file on disk is the whole input - // to the fingerprint a response reports. - function storedVersion(name: string) { - return computeContentHash(readFileSync(cardFile(name), 'utf8')); - } - function patchPersonBody(firstName: string) { return { data: { @@ -105,12 +97,16 @@ module(basename(import.meta.filename), function () { }; } - function incrementalIndexEvents(messages: MatrixEvent[]) { + // Both halves of an incremental index pass. The initiation event is + // broadcast per written file *before* indexing runs, so it is the earliest + // signal that something was staged — a test asserting a write was refused + // has to count it, or it only checks that the refusal failed to finish. + function indexEvents(messages: MatrixEvent[], indexType: string) { return messages.filter( (m) => m.type === APP_BOXEL_REALM_EVENT_TYPE && m.content.eventName === 'index' && - m.content.indexType === 'incremental', + m.content.indexType === indexType, ); } @@ -283,30 +279,47 @@ module(basename(import.meta.filename), function () { 'the conditional write is refused', ); - // The control: a write the realm does accept, made after the refused - // one and in the same window. Its event is what proves the window is - // one an event can arrive in — without it, "no event" would also be - // satisfied by a listener that sees nothing at all. The refused write - // came first, so anything it broadcast is already visible by the time - // this one's event is. + // The control is a write to a DIFFERENT card. Its event proves the + // window is one an event can arrive in — without a control, "no + // event" is equally satisfied by a listener that sees nothing at all. + // Aiming it elsewhere is what makes the two outcomes distinguishable: + // a control over the same card with the same body would be a no-op if + // the refused write had landed, and the commit writes nothing and + // queues nothing for a no-op, so the counts would agree either way. let accepted = await request - .patch('/person-1') + .patch('/person-2') .send(patchPersonBody('Paper')) .set('Accept', 'application/vnd.card+json'); assert.strictEqual(accepted.status, 200, 'the control write succeeds'); await waitForIncrementalIndexEvent(getMessagesSince, since); - let events = incrementalIndexEvents(await getMessagesSince(since)); - assert.strictEqual( - events.length, - 1, - 'exactly one write in the window reached the index', - ); - assert.deepEqual( - (events[0].content as { invalidations?: string[] }).invalidations, - [`${realmURL.href}person-1`], - 'and it is the one the realm accepted', - ); + let messages = await getMessagesSince(since); + // Both halves are counted: the initiation event is broadcast before + // indexing runs, so a refusal that staged bytes shows up there first. + for (let indexType of ['incremental-index-initiation', 'incremental']) { + let events = indexEvents(messages, indexType); + assert.strictEqual( + events.length, + 1, + `exactly one ${indexType} event in the window`, + ); + let content = events[0].content as { + invalidations?: string[]; + updatedFile?: string; + }; + let named = [ + ...(content.invalidations ?? []), + ...(content.updatedFile ? [content.updatedFile] : []), + ].join(' '); + assert.true( + named.includes('person-2'), + `the ${indexType} event is the control's`, + ); + assert.false( + named.includes('person-1'), + `nothing in the ${indexType} event names the refused card`, + ); + } }); test('a DELETE naming a validator the card has moved past is refused, and removes nothing', async function (assert) { @@ -426,211 +439,6 @@ module(basename(import.meta.filename), function () { .set('Accept', 'application/vnd.card+json'); assert.strictEqual(removal.status, 204, 'and so is the removal'); }); - - test('a PATCH reports the version of the file it wrote, and it moves with the bytes', async function (assert) { - let first = await request - .patch('/person-1') - .send(patchPersonBody('Van Gogh')) - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual(first.status, 200, `HTTP 200 status: ${first.text}`); - assert.strictEqual( - first.body?.data?.meta?.version, - storedVersion('person-1.json'), - 'the version is the fingerprint of the stored file', - ); - - let second = await request - .patch('/person-1') - .send(patchPersonBody('Paper')) - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual( - second.status, - 200, - `HTTP 200 status: ${second.text}`, - ); - assert.strictEqual( - second.body?.data?.meta?.version, - storedVersion('person-1.json'), - 'and again after the second write', - ); - assert.notStrictEqual( - second.body?.data?.meta?.version, - first.body?.data?.meta?.version, - 'a write that changes the card changes its version', - ); - }); - - test('a PATCH that changes nothing reports the version the card already held', async function (assert) { - let write = await request - .patch('/person-1') - .send(patchPersonBody('Van Gogh')) - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual(write.status, 200, `HTTP 200 status: ${write.text}`); - let version = write.body?.data?.meta?.version; - assert.ok(version, 'the write reports a version'); - - let noop = await request - .patch('/person-1') - .send(patchPersonBody('Van Gogh')) - .set('Accept', 'application/vnd.card+json'); - - assert.strictEqual(noop.status, 200, `HTTP 200 status: ${noop.text}`); - assert.strictEqual( - noop.body?.data?.meta?.version, - version, - 'a patch that leaves the file alone leaves the version alone', - ); - assert.strictEqual( - noop.body?.data?.meta?.version, - storedVersion('person-1.json'), - 'and it is still the fingerprint of what is stored', - ); - }); - - test('a POST reports the version of the file it created', async function (assert) { - let response = await request - .post('/') - .send({ - data: { - type: 'card', - attributes: { cardInfo: { name: 'Mango' } }, - meta: { - adoptsFrom: { - module: rri('@cardstack/base/card-api'), - name: 'CardDef', - }, - }, - }, - }) - .set('Accept', 'application/vnd.card+json'); - - assert.strictEqual( - response.status, - 201, - `HTTP 201 status: ${response.text}`, - ); - let id: string = response.body?.data?.id; - assert.ok(id, 'the create reports the id it minted'); - assert.strictEqual( - response.body?.data?.meta?.version, - storedVersion(`${id.slice(realmURL.href.length)}.json`), - 'the version is the fingerprint of the file it wrote', - ); - }); - - test('a GET reports the version the realm last wrote for the card', async function (assert) { - // Only the realm's own write path records a content hash, so a card - // that reached disk another way — as this realm's cards did — carries - // no version until it is next written. - let beforeAnyWrite = await request - .get('/person-1') - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual(beforeAnyWrite.status, 200, 'the card serves'); - assert.strictEqual( - beforeAnyWrite.body?.data?.meta?.version, - undefined, - 'and reports no version for a file the realm has never written', - ); - - let write = await request - .patch('/person-1') - .send(patchPersonBody('Van Gogh')) - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual(write.status, 200, `HTTP 200 status: ${write.text}`); - - let response = await request - .get('/person-1') - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual( - response.body?.data?.meta?.version, - storedVersion('person-1.json'), - 'the read reports the fingerprint of the stored file', - ); - assert.strictEqual( - response.body?.data?.meta?.version, - write.body?.data?.meta?.version, - 'which is the version the write that produced it reported', - ); - }); - - test("a linked card's change moves the validator and leaves the version alone", async function (assert) { - // The whole reason there are two: `hassan` links to `jade`, so writing - // `jade` re-indexes `hassan` and changes what a GET of it serves — but - // `hassan.json` is not rewritten, so the base an optimistic client - // holds for it must not move. - let write = await request - .patch('/hassan') - .send(patchFriendBody('Hassan')) - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual(write.status, 200, `HTTP 200 status: ${write.text}`); - - let before = await request - .get('/hassan') - .set('Accept', 'application/vnd.card+json'); - let etagBefore = before.get('etag') ?? ''; - let versionBefore = before.body?.data?.meta?.version; - assert.ok(etagBefore, 'the card carries a validator'); - assert.ok(versionBefore, 'and a version'); - - let linked = await request - .patch('/jade') - .send(patchFriendBody('Jade Vincent')) - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual( - linked.status, - 200, - `the linked card is written: ${linked.text}`, - ); - - let after = await request - .get('/hassan') - .set('Accept', 'application/vnd.card+json'); - assert.notStrictEqual( - after.get('etag'), - etagBefore, - 'the re-index moves the cache validator', - ); - assert.strictEqual( - after.body?.data?.meta?.version, - versionBefore, - 'and leaves the version, which describes only this card’s own file', - ); - }); - - test('a client echoing a version back does not persist it', async function (assert) { - let response = await request - .patch('/person-1') - .send({ - data: { - type: 'card', - attributes: { firstName: 'Van Gogh' }, - meta: { - adoptsFrom: { module: rri('./person.gts'), name: 'Person' }, - version: 'a-version-the-client-was-served', - }, - }, - }) - .set('Accept', 'application/vnd.card+json'); - - assert.strictEqual( - response.status, - 200, - `HTTP 200 status: ${response.text}`, - ); - let stored = JSON.parse( - readFileSync(cardFile('person-1.json'), 'utf8'), - ); - assert.strictEqual( - stored.data.meta.version, - undefined, - 'the stored file names no version', - ); - assert.strictEqual( - response.body?.data?.meta?.version, - storedVersion('person-1.json'), - 'and the response reports the one the realm computed', - ); - }); }); // A card read picks its link shape per request, and the shapes take diff --git a/packages/realm-server/tests/card-endpoints-test.ts b/packages/realm-server/tests/card-endpoints-test.ts index 5ee7cf2653e..1fe15eac391 100644 --- a/packages/realm-server/tests/card-endpoints-test.ts +++ b/packages/realm-server/tests/card-endpoints-test.ts @@ -2567,7 +2567,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -2703,7 +2702,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -2811,7 +2809,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -2853,7 +2850,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -3065,7 +3061,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4071,7 +4066,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4207,7 +4201,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4315,7 +4308,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4357,7 +4349,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4544,7 +4535,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, @@ -4630,7 +4620,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, diff --git a/packages/realm-server/tests/realm-endpoints-test.ts b/packages/realm-server/tests/realm-endpoints-test.ts index 00f6c7a4b5b..606bd4db6ec 100644 --- a/packages/realm-server/tests/realm-endpoints-test.ts +++ b/packages/realm-server/tests/realm-endpoints-test.ts @@ -954,7 +954,6 @@ module(basename(import.meta.filename), function () { delete json.data.meta.lastModified; delete json.data.meta.resourceCreatedAt; delete json.data.meta.generation; - delete json.data.meta.version; assert.strictEqual( response.get('X-boxel-realm-url'), testRealmHref, diff --git a/packages/runtime-common/card-operations/executors.ts b/packages/runtime-common/card-operations/executors.ts index 0737c7177ab..fb22c268702 100644 --- a/packages/runtime-common/card-operations/executors.ts +++ b/packages/runtime-common/card-operations/executors.ts @@ -706,15 +706,13 @@ export async function stageUpdate( // // Realm-managed keys never come from a patch: `realmInfo` and `realmURL` are // stamped by the realm serving the card, `screenshots` is joined from the - // prerendered manifest at serve time, `version` is the fingerprint of the - // bytes the realm last stored, and `type` is fixed by the document shape. A - // client echoing back what it was served must not persist any of them into - // the source file. + // prerendered manifest at serve time, and `type` is fixed by the document + // shape. A client echoing back what it was served must not persist any of + // them into the source file. delete (patch as { type?: unknown }).type; delete patch.meta.realmInfo; delete patch.meta.realmURL; delete patch.meta.screenshots; - delete patch.meta.version; promoteStagedLinks(patch, ctx); diff --git a/packages/runtime-common/file-meta.ts b/packages/runtime-common/file-meta.ts index fe9522ec8ec..0adbedfa750 100644 --- a/packages/runtime-common/file-meta.ts +++ b/packages/runtime-common/file-meta.ts @@ -53,63 +53,6 @@ export async function getContentMeta( }; } -// Everything `realm_file_meta` records about one file, in one row read. -// -// The two single-path readers above answer one column group each, so a caller -// that wants both pays two round-trips for one row. A card+json GET is exactly -// that caller: it reports the file's creation time as `x-created` and its -// content hash as the card's `meta.version`. -export async function getFileMeta( - db: DBAdapter, - realmURL: string, - localPath: string, -): Promise<{ - createdAt: number | undefined; - contentHash: string | undefined; - contentSize: number | undefined; -}> { - let absent = { - createdAt: undefined, - contentHash: undefined, - contentSize: undefined, - }; - if (!db) { - return absent; - } - let rows = await query(db, [ - 'SELECT created_at, content_hash, content_size FROM realm_file_meta WHERE realm_url =', - param(realmURL), - 'AND file_path =', - param(localPath), - 'LIMIT 1', - ]); - if (!rows || rows.length === 0) { - return absent; - } - let createdAt = rows[0]['created_at']; - let contentHash = rows[0]['content_hash']; - let contentSize = rows[0]['content_size']; - return { - // Each column is absent on its own: indexing inserts `created_at` alone - // for a file that reached disk outside the realm's write API, and only - // the write path ever fills the hash columns, so a row can hold either - // without the other. - createdAt: - createdAt == null - ? undefined - : typeof createdAt === 'string' - ? parseInt(createdAt) - : Number(createdAt), - contentHash: contentHash == null ? undefined : String(contentHash), - contentSize: - contentSize == null - ? undefined - : typeof contentSize === 'string' - ? parseInt(contentSize) - : Number(contentSize), - }; -} - // Reads created_at + content hash/size for many paths in a single query. Only // paths with a persisted row are returned; a caller treats an absent path as // "no persisted meta for this file" and falls back to its own per-path read. diff --git a/packages/runtime-common/file-serializer.ts b/packages/runtime-common/file-serializer.ts index 8875d9dca99..cc59dd6bfdb 100644 --- a/packages/runtime-common/file-serializer.ts +++ b/packages/runtime-common/file-serializer.ts @@ -122,10 +122,6 @@ export default async function serialize({ // the doc was GET from) — persisting an echo would pin a copied card's // source file to the original's captures. delete result.data.meta.screenshots; - // The fingerprint of these very bytes, reported alongside them when the card - // is served. Writing it into the file it describes would make the file name - // a version it no longer holds the moment it lands. - delete result.data.meta.version; delete result.included; delete result.data.links; result.data.type = 'card'; diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 3a1ff5042b5..054932464e7 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -87,7 +87,6 @@ import { removeFileMeta, getCreatedTime, getContentMeta, - getFileMeta, getFileMetaForPaths, } from './file-meta.ts'; import { @@ -7739,11 +7738,6 @@ export class Realm { }, }); } - // The version the commit just minted, reported by the write that produced - // it rather than read back out of `realm_file_meta` — a second read would - // answer for whatever the file holds now, which after a concurrent write - // is not what this response carries. - doc.data.meta.version = result.meta.version; this.#serveInstanceIdsAsRRI(doc); return createResponse({ body: JSON.stringify(doc, null, 2), @@ -7765,11 +7759,16 @@ export class Realm { // write stages nothing, enqueues no indexing and broadcasts no event. // // The comparand is the card's `ETag` — the same validator a `GET` of it - // hands out and an `If-None-Match` is compared against — not the card's - // `version`. That is what an HTTP conditional request names, and it is the - // stricter of the two: it moves when the served document may differ, so a - // client that revalidated its copy and then wrote holds a validator that - // still describes what it read. + // hands out and an `If-None-Match` is compared against. That is what an HTTP + // conditional request names, and it is the validator a client actually + // holds, since it is the only one a read gives out. + // + // It is the broader of the realm's two fingerprints and the later of them. + // Broader because it moves whenever the served document may differ, a linked + // card's re-index included, where the stored file's hash moves only when + // this card's own bytes do. Later because it is built from `indexed_at`, + // which moves after the bytes rather than with them — which is why the drain + // below is part of the precondition rather than an optimization. // // Comparison is `ifNoneMatchMatches`': `*`, comma lists, and the `W/` prefix // ignored on both sides. RFC 9110 §13.1.1 asks for strong comparison here @@ -7794,6 +7793,33 @@ export class Realm { if (ifMatch.trim() === '*') { return undefined; } + // Drain indexing already in flight before reading the row this rests on. + // Without it the precondition consults a source that lags the bytes by + // design: a commit writes the file and records its hash before it indexes, + // and a `skip-index-wait` write defers indexing to the worker entirely, so + // for that whole window the row still carries the pre-write `indexed_at` + // and a validator the caller has already been overtaken by still matches. + // A wrong accept there is the write this header exists to refuse, and it + // needs no concurrency in the request to happen. + // + // The cost lands on the requests that opt in by sending a concrete + // `If-Match`, and for most of them it is not new: a write that waits for + // its own indexing drains at the commit anyway, so this moves that wait + // earlier rather than adding one. + // + // Except for the callers that must never wait. A write made from inside a + // render, and one that asked for its indexing to be deferred, skip the + // drain here for the same reason they skip it at the commit: the job it + // would wait on needs the render slot the caller is holding, so waiting + // deadlocks rather than delays. Those are the realm's own machinery rather + // than a client reconciling an edit, and the precondition they get is the + // weaker one the unwaited row can support. + if ( + !isDuringPrerenderRequest(request) && + !isSkipIndexWaitRequest(request) + ) { + await this.incrementalIndexing(); + } await this.getRealmInfo(); let entry = await this.#realmIndexQueryEngine.instance(url, { includeErrors: true, @@ -7947,7 +7973,6 @@ export class Realm { }); } let created = result.meta.created; - let version = result.meta.version; let readEntry = async (skipQueryBackedExpansion: boolean) => await this.#realmIndexQueryEngine.cardDocument(new URL(instanceURL), { loadLinks: true, @@ -7969,10 +7994,6 @@ export class Realm { // the one the index recorded for it. lastModified: unchanged.doc.data.meta.lastModified ?? lastModified, created, - // Nothing was written, so this is the version the card already held - // — which is the point of reporting it: a patch that changes nothing - // leaves a client's base where it was. - version, requestContext, }); } @@ -7991,7 +8012,6 @@ export class Realm { } lastModified = result?.meta.lastModified ?? lastModified; created = result?.meta.created ?? created; - version = result?.meta.version ?? version; } if (answerFromEcho) { // See serializedInstanceEcho: the write indexed deferred, so there is @@ -8011,7 +8031,6 @@ export class Realm { instanceURL, lastModified, ); - doc.data.meta.version = version; this.#serveInstanceIdsAsRRI(doc); return createResponse({ body: JSON.stringify(doc, null, 2), @@ -8033,7 +8052,6 @@ export class Realm { localPath, lastModified, created, - version, requestContext, }); } @@ -8059,7 +8077,6 @@ export class Realm { meta: { ...(stored.data.meta ?? {}), lastModified, - version, }, }, }) as SingleCardDocument; @@ -8096,26 +8113,19 @@ export class Realm { localPath, lastModified, created, - version, requestContext, }: { instanceURL: string; localPath: LocalPath; lastModified: number | null; created: number | null; - version: string; requestContext: RequestContext; }, ): Promise { let doc: SingleCardDocument = merge({}, entry.doc, { data: { links: { self: instanceURL }, - // The version the commit reported for the file it left behind, which - // is the one this response's bytes describe. The index row this - // document came from carries no fingerprint of the source, and reading - // one back would answer for the file as it stands now rather than as - // this write left it. - meta: { lastModified, version }, + meta: { lastModified }, }, }); // The PATCH echo carries the joined `meta.screenshots` like a GET does — @@ -8833,7 +8843,7 @@ export class Realm { ); } let { document, headers, queryBacked } = result; - if (!isSingleCardDocument(document)) { + if (document.data.type === 'file-meta') { // A file's metadata is derived from its bytes, so there is no index row // to validate it against and nothing here to cache. return { kind: 'file-meta', body: JSON.stringify(document, null, 2) }; @@ -8849,31 +8859,9 @@ export class Realm { return { kind: 'redirect', foundPath }; } - // Prefer created_at from DB for instance JSON. The card's `version` comes - // out of the same row, so reporting it costs this read's column list - // rather than a second round-trip. + // Prefer created_at from DB for instance JSON let pathForDb = this.paths.local(url) + '.json'; - let { createdAt, contentHash } = await this.storedFileMetaFor(pathForDb); - // The card's write identity: the fingerprint of the `.json` the realm - // stores, which moves only when that file is rewritten. Deliberately not - // the `ETag` beside it — that one is the HTTP cache validator and tracks - // the served *document*, so it also moves when a card this one links to is - // re-indexed. A client holding an optimistic edit reconciles on `version`. - // - // Only the realm's own write path records a hash, so a card that reached - // disk some other way — a deployed or seeded realm, a fixture copied into - // place — has none until it is next written, and the key is absent rather - // than computed: hashing the file to answer a read would put a bounded but - // real file read on every uncached GET of every such card. - // - // Read after the document and reported with it, so the pair a caller acts - // on is the pair one assembly produced. The response cache keeps them - // together for the same reason: it retains the serialized body, so a - // cached document and the version it was assembled beside are served - // together or not at all. - if (contentHash !== undefined) { - card.data.meta.version = contentHash; - } + let createdAt = await this.getCreatedTime(pathForDb); // deps + indexedAt come off the assembly the read reports, not off the // early peek: the two see different snapshots when a write lands between // them, and a validator has to describe the bytes it is sent with. @@ -9237,18 +9225,6 @@ export class Realm { return getCreatedTime(this.#dbAdapter, this.url, path); } - // The whole `realm_file_meta` row for a path, for a caller that reports more - // than one of its columns and would otherwise read the row once per column. - private async storedFileMetaFor(path: LocalPath): Promise<{ - createdAt: number | undefined; - contentHash: string | undefined; - }> { - if (!this.#dbAdapter) { - return { createdAt: undefined, contentHash: undefined }; - } - return getFileMeta(this.#dbAdapter, this.url, path); - } - private async directoryEntries( url: URL, ): Promise<{ name: string; kind: Kind; path: LocalPath }[] | undefined> { diff --git a/packages/runtime-common/resource-types.ts b/packages/runtime-common/resource-types.ts index b672ea848d4..f29bbbd849b 100644 --- a/packages/runtime-common/resource-types.ts +++ b/packages/runtime-common/resource-types.ts @@ -119,19 +119,6 @@ export type CardResourceMeta = Meta & { // writes like `realmInfo`/`realmURL`. The `screenshotURLs` getter on // CardDef/FileDef reads this. screenshots?: ScreenshotsMeta; - // The fingerprint of the `.json` file the realm stores for this card — the - // card's write identity, and the base an optimistic client reconciles - // against. It moves only when that file is rewritten, so it is stable across - // the re-indexes a linked card's change causes. - // - // Distinct from the HTTP `ETag` on the same response, which tracks the - // served document rather than the stored file and is therefore the wrong - // thing to hold an edit against — though it is what a conditional write - // (`If-Match`) names, because that is a cache validator's job. - // - // Serve-time output like the keys above: stripped from a write's payload, so - // a client echoing back what it was served never persists one. - version?: string; }; export type FileMetaResourceResourceMeta = Meta & { From 9543c99fe6e2ffc3307274fcdea77f0bf9c635ea Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Wed, 16 Sep 2026 22:53:13 -0400 Subject: [PATCH 04/14] Open the event window after the setup write's own events have landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A write's index events are broadcast without await ordering, so a `PATCH` returns — having waited for its index job — while its incremental event is still in flight. A window opened immediately after one catches that event and reads it as the next write's, which named the same card the refusal names. The window now opens once the setup write's events are visible, and the assertion is which card the window's events name rather than how many there are. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-conditional-write-test.ts | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts index 07fed1b1355..a7646a66273 100644 --- a/packages/realm-server/tests/card-conditional-write-test.ts +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -261,12 +261,21 @@ module(basename(import.meta.filename), function () { .get('/person-1') .set('Accept', 'application/vnd.card+json'); let staleEtag = read.get('etag') ?? ''; + let setupSince = Date.now(); let moved = await request .patch('/person-1') .send(patchPersonBody('Van Gogh')) .set('Accept', 'application/vnd.card+json'); assert.strictEqual(moved.status, 200, 'the card is moved past it'); + // Drain the setup write's own events before the window opens. A + // write's index events are broadcast without await ordering, so the + // `PATCH` above can return — having waited for its index job — while + // its incremental event is still on its way. Opening the window + // without flushing it first lets that event land inside the window and + // be read as the refusal's, which names the same card. + await waitForIncrementalIndexEvent(getMessagesSince, setupSince); + let since = Date.now(); let refused = await request .patch('/person-1') @@ -294,30 +303,31 @@ module(basename(import.meta.filename), function () { await waitForIncrementalIndexEvent(getMessagesSince, since); let messages = await getMessagesSince(since); - // Both halves are counted: the initiation event is broadcast before - // indexing runs, so a refusal that staged bytes shows up there first. + // Both halves are checked: the initiation event is broadcast per + // written file *before* indexing runs, so a refusal that staged + // anything shows up there first. + // + // The assertion is about which card the window's events name, not how + // many there are: the control's presence proves the window is one an + // event can arrive in, and the refused card's absence is the claim. for (let indexType of ['incremental-index-initiation', 'incremental']) { - let events = indexEvents(messages, indexType); - assert.strictEqual( - events.length, - 1, - `exactly one ${indexType} event in the window`, - ); - let content = events[0].content as { - invalidations?: string[]; - updatedFile?: string; - }; - let named = [ - ...(content.invalidations ?? []), - ...(content.updatedFile ? [content.updatedFile] : []), - ].join(' '); + let named = indexEvents(messages, indexType).map((event) => { + let content = event.content as { + invalidations?: string[]; + updatedFile?: string; + }; + return [ + ...(content.invalidations ?? []), + ...(content.updatedFile ? [content.updatedFile] : []), + ].join(' '); + }); assert.true( - named.includes('person-2'), - `the ${indexType} event is the control's`, + named.some((urls) => urls.includes('person-2')), + `the control's ${indexType} event arrived in the window`, ); assert.false( - named.includes('person-1'), - `nothing in the ${indexType} event names the refused card`, + named.some((urls) => urls.includes('person-1')), + `no ${indexType} event in the window names the refused card`, ); } }); From 77826a65eda1a211e10ffe12e09bf9713b7c82ec Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 03:22:11 -0400 Subject: [PATCH 05/14] Evaluate the conditional write's precondition inside the commit's lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A precondition checked before `commitBatch` is only as sound as the realm is uncontended: the write lock is taken inside that call, so a request whose check passed can queue behind another writer's entire write and then stage against state that moved. That is the contended case a conditional write exists for, so the check held exactly where it was needed least. `CommitBatchOptions` gains a `precondition` the coordinator invokes inside the lock, after its drain and before anything stages. The realm keeps what the check is — an `If-Match` compares an `ETag` built from index and realm-info state the coordinator has no business assembling — and the coordinator owns only when it runs. A refusal travels as the operation failure it throws, so the 412 reaches the caller by the path every other card-write refusal takes. The two tests that pin this read the stub's lock depth and drain count from inside the precondition, because a response carries the same status and body whether the check ran inside the lock, outside it, or not at all. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-operations-batch-test.ts | 113 +++++++++++++ .../card-operations/coordinator.ts | 17 ++ packages/runtime-common/error.ts | 24 --- packages/runtime-common/realm.ts | 157 ++++++++---------- 4 files changed, 196 insertions(+), 115 deletions(-) diff --git a/packages/realm-server/tests/card-operations-batch-test.ts b/packages/realm-server/tests/card-operations-batch-test.ts index c64dba144de..d87f913bb14 100644 --- a/packages/realm-server/tests/card-operations-batch-test.ts +++ b/packages/realm-server/tests/card-operations-batch-test.ts @@ -5,6 +5,7 @@ import { basename } from 'path'; import { commitBatch, isOperationFailure, + OperationFailure, type BatchCore, type BatchEntry, type OperationDefinition, @@ -3008,5 +3009,117 @@ module(basename(import.meta.filename), function () { }); assert.strictEqual(commits.length, 0, 'nothing is committed'); }); + + // A caller's precondition is only binding where it runs. Evaluated before + // `commitBatch`, a check can pass and the request then queue behind + // another writer's whole write, so it holds only while the realm is + // uncontended — which inverts what a conditional write is for. These pin + // the placement rather than the outcome: a response asserts the same + // status and body whether the check ran inside the lock, outside it, or + // not at all, so no endpoint test can tell the three apart. + test('a precondition runs inside the lock, after the drain, before anything is staged', async function (assert) { + let s = stub({ + stored: { + 'person-1.json': cardFile({ firstName: 'Original' }, PERSON), + }, + }); + let observed: + | { lockDepth: number; drains: number; commits: number } + | undefined; + await commitBatch( + s.core, + [ + { + op: 'update', + href: `${REALM}person-1`, + document: { + data: { + type: 'card', + attributes: { firstName: 'Updated' }, + meta: { adoptsFrom: PERSON }, + }, + }, + }, + ], + { + precondition: async () => { + observed = { + lockDepth: s.lockDepth(), + drains: s.drainCount(), + commits: s.commits.length, + }; + }, + }, + ); + + assert.ok(observed, 'the precondition was called'); + assert.strictEqual( + observed?.lockDepth, + 1, + 'with the write lock held, so the state it reads cannot move under it', + ); + assert.strictEqual( + observed?.drains, + 1, + 'after the drain, so the index it reads is current with the bytes', + ); + assert.strictEqual( + observed?.commits, + 0, + 'and before anything was staged', + ); + assert.strictEqual(s.commits.length, 1, 'the write then proceeds'); + }); + + test('a precondition that refuses writes nothing and commits nothing', async function (assert) { + let { core, commits } = stub({ + stored: { + 'person-1.json': cardFile({ firstName: 'Original' }, PERSON), + }, + }); + let failure = await commitBatch( + core, + [ + { + op: 'update', + href: `${REALM}person-1`, + document: { + data: { + type: 'card', + attributes: { firstName: 'Updated' }, + meta: { adoptsFrom: PERSON }, + }, + }, + }, + ], + { + precondition: async () => { + throw new OperationFailure({ + id: `${REALM}person-1`, + status: 412, + code: 'version-conflict', + title: 'Precondition Failed', + detail: 'the card is not the one the caller saw', + }); + }, + }, + ).then( + () => undefined, + (err: unknown) => (isOperationFailure(err) ? err.error : err), + ); + + assert.deepEqual( + failure, + { + id: `${REALM}person-1`, + status: 412, + code: 'version-conflict', + title: 'Precondition Failed', + detail: 'the card is not the one the caller saw', + }, + "the caller's own refusal is what the batch fails with, status included", + ); + assert.strictEqual(commits.length, 0, 'and nothing is committed'); + }); }); }); diff --git a/packages/runtime-common/card-operations/coordinator.ts b/packages/runtime-common/card-operations/coordinator.ts index f1b88300bdd..c4bccdd94ed 100644 --- a/packages/runtime-common/card-operations/coordinator.ts +++ b/packages/runtime-common/card-operations/coordinator.ts @@ -204,6 +204,18 @@ export interface CommitBatchOptions { // requirement rather than a preference: the job it would wait on needs the // render slot that caller is holding, so waiting deadlocks. waitForIndex?: boolean; + // A caller's own precondition, run inside the write lock and before + // anything is staged. It exists because a precondition evaluated before + // `commitBatch` is not binding: the lock is acquired here, so a request + // whose check passed can then queue behind another writer's whole write and + // proceed against state that moved. Throwing from here refuses the batch + // with nothing staged, nothing enqueued and no event broadcast, and the + // thrown failure's status is what the caller answers with. + // + // The realm keeps the *content* of the check — an `If-Match` compares a + // card's `ETag`, which is built from index and realm-info state the + // coordinator has no business assembling. This owns only when it runs. + precondition?: () => Promise; // Report the bytes each entry's primary file now holds, on the entry's // `meta.storedContent`. Off by default: a batch's results say what a card // is and what version it holds, and a caller wanting its document reads it @@ -284,6 +296,11 @@ export async function commitBatch( if (opts.waitForIndex !== false && entries.some(stagesContent)) { await core.drainIndexing(); } + // After the drain, so the state a precondition reads is the realm as this + // batch is about to change it, and before staging, so a refusal costs + // nothing but the lock it already holds. + await opts.precondition?.(); + // Every `lid` in the batch resolves to a URL before any executor runs. A // created card's file is named after its `lid`, so its URL is path math // over the type it adopts — no read and no write — which is what lets an diff --git a/packages/runtime-common/error.ts b/packages/runtime-common/error.ts index a1fa2bd8cf3..c80421e69bb 100644 --- a/packages/runtime-common/error.ts +++ b/packages/runtime-common/error.ts @@ -940,30 +940,6 @@ export function notAcceptable( ); } -// The card at the URL is not the one the request's `If-Match` names, so the -// write is refused before anything is staged. The status is the whole answer a -// conditional client acts on — it re-reads the card and decides what to do -// about the edit it was holding — so the body only names which validator was -// asked for. -export function preconditionFailed({ - request, - requestContext, - id, -}: { - request: Request; - requestContext: RequestContext; - id: string; -}): Response { - return responseWithError( - new CardError( - `${request.method} of ${id} requires the card to match ` + - `If-Match: ${request.headers.get('if-match')}, and it does not`, - { status: 412, id }, - ), - requestContext, - ); -} - export function badRequest({ message, requestContext, diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 054932464e7..779bb66087f 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -96,7 +96,6 @@ import { notAcceptable, methodNotAllowed, badRequest, - preconditionFailed, CardError, responseWithError, formattedError, @@ -173,7 +172,7 @@ import { isHeadResult, isOperationFailure, isSourceResult, - type OperationFailure, + OperationFailure, type OperationRequest, type OperationResult, type OperationSourceResult, @@ -7753,22 +7752,28 @@ export class Realm { }); } - // A conditional write: the 412 a `PATCH` or `DELETE` carrying `If-Match` - // answers when the card is no longer the one the caller saw, or `undefined` - // for a request that may proceed. Called before the commit, so a refused - // write stages nothing, enqueues no indexing and broadcasts no event. + // A conditional write's precondition: throws a 412 when the card is no + // longer the one the caller saw, and returns for a request that may + // proceed. Handed to `commitBatch` rather than called before it, because it + // is only binding inside the write lock — a check made before that lock can + // pass and then queue behind another writer's whole write, which is exactly + // the contended case the header exists for. The coordinator runs it after + // its drain and before anything is staged, so a refusal writes nothing, + // enqueues nothing and broadcasts nothing; the drain is the coordinator's, + // so a caller that must not wait on indexing still does not. // // The comparand is the card's `ETag` — the same validator a `GET` of it - // hands out and an `If-None-Match` is compared against. That is what an HTTP + // hands out and an `If-None-Match` is checked against. That is what an HTTP // conditional request names, and it is the validator a client actually // holds, since it is the only one a read gives out. // // It is the broader of the realm's two fingerprints and the later of them. - // Broader because it moves whenever the served document may differ, a linked - // card's re-index included, where the stored file's hash moves only when - // this card's own bytes do. Later because it is built from `indexed_at`, - // which moves after the bytes rather than with them — which is why the drain - // below is part of the precondition rather than an optimization. + // Broader because it moves whenever the served document may differ, a + // linked card's re-index included, where the stored file's hash moves only + // when this card's own bytes do. Later because it is built from + // `indexed_at`, which moves after the bytes rather than with them — which + // is why running inside the lock, after the drain, is what makes it mean + // anything. // // Comparison is `ifNoneMatchMatches`': `*`, comma lists, and the `W/` prefix // ignored on both sides. RFC 9110 §13.1.1 asks for strong comparison here @@ -7781,71 +7786,53 @@ export class Realm { // can offer no validator for — never indexed, or an `ETag` suppressed // because the document depends on another realm — fails the precondition: // "this is the card you saw" is a claim the realm cannot make. - async #conditionalWriteRefusal( + #conditionalWrite( request: Request, url: URL, - requestContext: RequestContext, - ): Promise { + ): (() => Promise) | undefined { let ifMatch = request.headers.get('if-match'); - if (!ifMatch) { - return undefined; - } - if (ifMatch.trim() === '*') { - return undefined; - } - // Drain indexing already in flight before reading the row this rests on. - // Without it the precondition consults a source that lags the bytes by - // design: a commit writes the file and records its hash before it indexes, - // and a `skip-index-wait` write defers indexing to the worker entirely, so - // for that whole window the row still carries the pre-write `indexed_at` - // and a validator the caller has already been overtaken by still matches. - // A wrong accept there is the write this header exists to refuse, and it - // needs no concurrency in the request to happen. - // - // The cost lands on the requests that opt in by sending a concrete - // `If-Match`, and for most of them it is not new: a write that waits for - // its own indexing drains at the commit anyway, so this moves that wait - // earlier rather than adding one. - // - // Except for the callers that must never wait. A write made from inside a - // render, and one that asked for its indexing to be deferred, skip the - // drain here for the same reason they skip it at the commit: the job it - // would wait on needs the render slot the caller is holding, so waiting - // deadlocks rather than delays. Those are the realm's own machinery rather - // than a client reconciling an edit, and the precondition they get is the - // weaker one the unwaited row can support. - if ( - !isDuringPrerenderRequest(request) && - !isSkipIndexWaitRequest(request) - ) { - await this.incrementalIndexing(); - } - await this.getRealmInfo(); - let entry = await this.#realmIndexQueryEngine.instance(url, { - includeErrors: true, - }); - if (entry?.type !== 'instance' || this.hasForeignRealmDeps(entry.deps)) { - return preconditionFailed({ request, requestContext, id: url.href }); - } - // Every validator the realm would hand out for this card as it stands. - // A card+json read picks its link shape per request — the setting can - // differ between the read that gave the client its validator and this - // write — and the shapes take different variants of one validator so a - // client holding either is not 304'd to the other. That distinction is - // about representations; this question is about the card, and all of - // these describe the same card at the same `indexed_at`. Refusing a write - // because the read that preceded it answered in the narrower shape would - // refuse on a server setting rather than on anything the caller did. - let realmInfoHash = this.getCachedRealmInfoHash(); - let screenshots = screenshotsEtagFingerprint(entry.screenshots); - let issued = [ - buildCardJsonEtag(entry.indexedAt, realmInfoHash, screenshots, false), - buildCardJsonEtag(entry.indexedAt, realmInfoHash, screenshots, true), - ]; - if (issued.some((etag) => etag && ifNoneMatchMatches(ifMatch, etag))) { + if (!ifMatch || ifMatch.trim() === '*') { return undefined; } - return preconditionFailed({ request, requestContext, id: url.href }); + let refuse = (): never => { + throw new OperationFailure({ + id: url.href, + status: 412, + code: 'version-conflict', + title: 'Precondition Failed', + detail: + `${request.method} of ${url.href} requires the card to match ` + + `If-Match: ${ifMatch}, and it does not`, + }); + }; + return async () => { + await this.getRealmInfo(); + let entry = await this.#realmIndexQueryEngine.instance(url, { + includeErrors: true, + }); + if (entry?.type !== 'instance' || this.hasForeignRealmDeps(entry.deps)) { + refuse(); + } + // Every validator the realm would hand out for this card as it stands. + // A card+json read picks its link shape per request — the setting can + // differ between the read that gave the client its validator and this + // write — and the shapes take different variants of one validator so a + // client holding either is not 304'd to the other. That distinction is + // about representations; this question is about the card, and all of + // these describe the same card at the same `indexed_at`. Refusing a + // write because the read that preceded it answered in the narrower + // shape would refuse on a server setting rather than on anything the + // caller did. + let realmInfoHash = this.getCachedRealmInfoHash(); + let screenshots = screenshotsEtagFingerprint(entry!.screenshots); + let issued = [ + buildCardJsonEtag(entry!.indexedAt, realmInfoHash, screenshots, false), + buildCardJsonEtag(entry!.indexedAt, realmInfoHash, screenshots, true), + ]; + if (!issued.some((etag) => etag && ifNoneMatchMatches(ifMatch, etag))) { + refuse(); + } + }; } private async patchCardInstance( @@ -7895,17 +7882,10 @@ export class Realm { } } - // Read after the body, so a payload the realm would have refused anyway - // still gets the 400 that names what is wrong with it, and a 412 means - // only that the card moved. - let refusal = await this.#conditionalWriteRefusal( - request, - new URL(instanceURL), - requestContext, - ); - if (refusal) { - return refusal; - } + // Built after the body is validated, so a payload the realm would have + // refused anyway still gets the 400 naming what is wrong with it and a + // 412 means only that the card moved. It runs inside the commit's lock. + let precondition = this.#conditionalWrite(request, new URL(instanceURL)); // The merge belongs to the update this stages: arrays replacing rather // than merging into what is there, the realm-managed `meta` keys a client @@ -7950,6 +7930,7 @@ export class Realm { // The same reading a create takes of a side-load claiming another // realm: the edge is stored empty and the patch succeeds. foreignSideLoadLink: 'leave', + ...(precondition ? { precondition } : {}), }, ) )[0]; @@ -9188,14 +9169,7 @@ export class Realm { if (await this.openFileForMetadata(localPath)) { return methodNotAllowed(request, requestContext); } - let refusal = await this.#conditionalWriteRefusal( - request, - url, - requestContext, - ); - if (refusal) { - return refusal; - } + let precondition = this.#conditionalWrite(request, url); try { // Whether there is a card here is settled by the stored file, read // inside the same lock the removal happens under. That is what makes a @@ -9206,6 +9180,7 @@ export class Realm { ...(requestContext.authenticatedUser ? { actor: requestContext.authenticatedUser } : {}), + ...(precondition ? { precondition } : {}), }); } catch (err: unknown) { return this.#cardWriteRefusal(err, request, requestContext, { From 7572ee57c724136a2f6a7bc36fa222d916881d88 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 04:03:16 -0400 Subject: [PATCH 06/14] Drain a precondition's indexing across replicas, and for removals too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write-path drain waited on an in-memory map of the jobs this process enqueued, so a peer replica's pending index job was invisible to it. A write that deferred its indexing on one replica therefore left another replica reading a row that still described the pre-write card — and a conditional write comparing an index-derived validator against that row accepts a request it exists to refuse. It now also waits on the realm's indexing lane in the shared jobs table, which every replica writes to. Scoped to the job types the write path races. A from-scratch pass reads files independently of realm-server writes, so waiting on one would park every write behind a system-wide reindex — the same exclusion the in-memory gate makes, now stated once and read by both. The drain was also skipped entirely for a batch that stages nothing, which is every removal. A removal resolves no definition, so it has no use for the freshness the drain was written for; a removal's precondition reads indexed state and has every use for it. A caller that brought one now forces the wait whatever the batch stages. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-operations-batch-test.ts | 26 +++++++++++ .../card-operations/coordinator.ts | 10 ++++- packages/runtime-common/jobs/indexing.ts | 43 ++++++++++++++++--- packages/runtime-common/realm.ts | 24 +++++++++++ 4 files changed, 97 insertions(+), 6 deletions(-) diff --git a/packages/realm-server/tests/card-operations-batch-test.ts b/packages/realm-server/tests/card-operations-batch-test.ts index d87f913bb14..a813366319a 100644 --- a/packages/realm-server/tests/card-operations-batch-test.ts +++ b/packages/realm-server/tests/card-operations-batch-test.ts @@ -3071,6 +3071,32 @@ module(basename(import.meta.filename), function () { assert.strictEqual(s.commits.length, 1, 'the write then proceeds'); }); + test('a delete carrying a precondition drains first, though it stages nothing', async function (assert) { + // A removal stages no content, so the batch would otherwise skip the + // drain — and a removal is the one verb whose precondition is answered + // entirely from indexed state, with no staged bytes to read instead. An + // undrained index still spells the pre-write validator, so a stale + // `If-Match` would match and the newer file would be removed. + let s = stub({ + stored: { + 'person-1.json': cardFile({ firstName: 'Original' }, PERSON), + }, + }); + let drainsWhenChecked: number | undefined; + await commitBatch(s.core, [{ op: 'delete', href: `${REALM}person-1` }], { + precondition: async () => { + drainsWhenChecked = s.drainCount(); + }, + }); + + assert.strictEqual( + drainsWhenChecked, + 1, + 'the drain ran before the precondition, though nothing staged', + ); + assert.strictEqual(s.commits.length, 1, 'and the removal then proceeds'); + }); + test('a precondition that refuses writes nothing and commits nothing', async function (assert) { let { core, commits } = stub({ stored: { diff --git a/packages/runtime-common/card-operations/coordinator.ts b/packages/runtime-common/card-operations/coordinator.ts index c4bccdd94ed..c621fc0b3a4 100644 --- a/packages/runtime-common/card-operations/coordinator.ts +++ b/packages/runtime-common/card-operations/coordinator.ts @@ -293,7 +293,15 @@ export async function commitBatch( // because this wait happens with the realm's write lock held — a removal // issued while a bulk import drains would park here holding it, with // every other writer queued behind. - if (opts.waitForIndex !== false && entries.some(stagesContent)) { + // A batch that stages nothing skips the drain on its own terms — a removal + // names a file and reads the bytes already there, resolving no definition + // — but a precondition reads *indexed* state, and a removal is exactly the + // verb whose precondition would otherwise be answered from an index that + // has not caught up with the bytes it is about to delete. So a caller that + // brought one forces the wait whatever the batch stages. + let readsIndexedState = + entries.some(stagesContent) || opts.precondition !== undefined; + if (opts.waitForIndex !== false && readsIndexedState) { await core.drainIndexing(); } // After the drain, so the state a precondition reads is the realm as this diff --git a/packages/runtime-common/jobs/indexing.ts b/packages/runtime-common/jobs/indexing.ts index 86e0ff9c9a2..c6c0d00aa13 100644 --- a/packages/runtime-common/jobs/indexing.ts +++ b/packages/runtime-common/jobs/indexing.ts @@ -6,7 +6,12 @@ import type { IncrementalDoneResult, IncrementalResult, } from '../tasks/indexer.ts'; -import { param, query, type PgPrimitive } from '../expression.ts'; +import { + param, + query, + type Expression, + type PgPrimitive, +} from '../expression.ts'; import type { DBAdapter } from '../db.ts'; import { baseRealm, baseRealmRRI } from '../constants.ts'; import { systemInitiatedPriority, userInitiatedPriority } from '../queue.ts'; @@ -200,10 +205,26 @@ export async function unbuiltIndexFailure( return typeof result === 'string' ? result : JSON.stringify(result); } +// The job types a realm's write path races against: the two that rewrite index +// rows for files someone just wrote. `from-scratch-index` is deliberately not +// among them — it reads files independently of realm-server writes and each row +// write is atomic, so blocking a `PATCH` on one would park user writes behind a +// system-wide reindex for as long as that takes. This is the cross-replica +// spelling of the scope `RealmIndexUpdater.incrementalIndexing()` keeps in +// memory, and the two must stay in step. +export const WRITE_RACING_INDEX_JOB_TYPES = ['incremental-index', 'copy-index']; + export async function awaitRealmIndexSettled( dbAdapter: DBAdapter, realmURL: string, - opts?: { timeoutMs?: number; pollIntervalMs?: number }, + opts?: { + timeoutMs?: number; + pollIntervalMs?: number; + // Narrows the lane to particular job types. Absent means the whole + // `indexing:` lane, which is what a caller wanting "all indexing has + // settled" (a publish, a readiness probe) asks for. + jobTypes?: string[]; + }, ): Promise { if (dbAdapter.kind !== 'pg') { return true; @@ -211,13 +232,25 @@ export async function awaitRealmIndexSettled( let timeoutMs = opts?.timeoutMs ?? 10_000; let pollIntervalMs = opts?.pollIntervalMs ?? 1000; + let jobTypes = opts?.jobTypes; let hasSettled = async () => { - let rows = await query(dbAdapter, [ + let expression: Expression = [ `SELECT 1 FROM jobs WHERE status = 'unfulfilled' AND concurrency_group =`, param(indexingConcurrencyGroup(realmURL)), - 'LIMIT 1', - ]); + ]; + if (jobTypes) { + expression.push('AND job_type IN', '('); + jobTypes.forEach((jobType, index) => { + if (index > 0) { + expression.push(','); + } + expression.push(param(jobType)); + }); + expression.push(')'); + } + expression.push('LIMIT 1'); + let rows = await query(dbAdapter, expression); return rows.length === 0; }; diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 779bb66087f..312793f7f62 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -6,6 +6,7 @@ import { INCREMENTAL_INDEX_JOB_TIMEOUT_SEC, prerenderSpawnedPriority, unbuiltIndexFailure, + WRITE_RACING_INDEX_JOB_TYPES, } from './jobs/indexing.ts'; import { awaitPublishedHtmlReady, @@ -4304,7 +4305,30 @@ export class Realm { localPath, ), drainIndexing: async () => { + // Two halves, because neither sees what the other does. The + // in-memory deferreds cover the jobs this process enqueued and + // nothing else — they are a `Map` on this replica's index updater — + // so a write taken by a peer replica, or one deferred to a worker, + // is invisible here. The jobs table is shared, so a query against + // the realm's indexing lane sees every replica's pending work. + // + // Without the second half the gate is only as good as the realm + // having one replica: a `skip-index-wait` write on replica A leaves + // A's index job pending while replica B, seeing nothing local to + // drain, reads a row that still describes the pre-write card. + // + // Scoped to the job types the write path actually races. A + // from-scratch pass reads files independently of realm-server + // writes, so waiting on one would park every write behind a + // system-wide reindex for as long as it takes — the same exclusion + // `incrementalIndexing()` makes in memory, which is why the two are + // defined against one list. await this.incrementalIndexing(); + if (this.#dbAdapter) { + await awaitRealmIndexSettled(this.#dbAdapter, this.url, { + jobTypes: WRITE_RACING_INDEX_JOB_TYPES, + }); + } }, isIgnored: (url) => this.isIgnored(url), // Narrowed to the two documents a program reads values from, each From 16ae09f71e704ca986f3a6bf9a2d32eec2d94fdc Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 04:25:07 -0400 Subject: [PATCH 07/14] Refuse a conditional write the realm cannot decide, and leave the drain alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cross-replica check belongs to the precondition, not to the shared drain. Riding the drain made every card write pay for a guarantee only a conditional one asked for, forced a removal to wait on an undeadlined in-memory gate while holding the write lock, and — because the drain discards its answer — turned a lane that would not settle into a silent accept, which is the congestion the check exists to catch. The precondition now runs its own deadlined query against the realm's indexing lane and refuses when it will not settle. An undecidable precondition answers 5xx rather than 412: nothing about the request is wrong, and repeating it is the remedy. It says so in the log, because a lane that never drains otherwise looks from here like a realm with nothing to refuse. A refusal also carries the card it is about again, which the rethrow branch had been dropping. The placement test now asserts nothing has been read to stage from, rather than nothing committed: staging reads the pre-state, so a hook that had slipped past it still saw zero commits and the test passed. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-operations-batch-test.ts | 41 ++++++---- .../card-operations/coordinator.ts | 10 +-- .../runtime-common/card-operations/types.ts | 6 ++ packages/runtime-common/jobs/indexing.ts | 2 +- packages/runtime-common/realm.ts | 77 +++++++++++++------ 5 files changed, 88 insertions(+), 48 deletions(-) diff --git a/packages/realm-server/tests/card-operations-batch-test.ts b/packages/realm-server/tests/card-operations-batch-test.ts index a813366319a..25f79b1b6b0 100644 --- a/packages/realm-server/tests/card-operations-batch-test.ts +++ b/packages/realm-server/tests/card-operations-batch-test.ts @@ -61,6 +61,12 @@ interface Stub { lockDepth: () => number; drainCount: () => number; readsOutsideLock: () => number; + // Every read of a stored file, in or out of the lock. `readsOutsideLock` + // answers where a read happened; this answers whether one has happened at + // all, which is what distinguishes "before anything was staged" from + // "before the commit" — staging reads the pre-state, and a hook that had + // slipped past it would still see no commits. + sourceReads: () => number; } interface StubOptions { @@ -117,6 +123,7 @@ function stub(opts: StubOptions = {}): Stub { let maxHeld = 0; let drains = 0; let readsOutsideLock = 0; + let sourceReads = 0; let core: BatchCore = { realmURL: REALM, @@ -131,10 +138,12 @@ function stub(opts: StubOptions = {}): Stub { }, async fileExists(localPath) { readsOutsideLock += held > 0 ? 0 : 1; + sourceReads++; return stored[localPath] !== undefined; }, async readSourceFile(localPath) { readsOutsideLock += held > 0 ? 0 : 1; + sourceReads++; let content = stored[localPath]; return content === undefined ? undefined @@ -142,6 +151,7 @@ function stub(opts: StubOptions = {}): Stub { }, async openSourceBytes(localPath) { readsOutsideLock += held > 0 ? 0 : 1; + sourceReads++; let content = stored[localPath]; if (content === undefined) { return undefined; @@ -244,6 +254,7 @@ function stub(opts: StubOptions = {}): Stub { lockDepth: () => maxHeld, drainCount: () => drains, readsOutsideLock: () => readsOutsideLock, + sourceReads: () => sourceReads, }; } @@ -3024,7 +3035,7 @@ module(basename(import.meta.filename), function () { }, }); let observed: - | { lockDepth: number; drains: number; commits: number } + | { lockDepth: number; drains: number; sourceReads: number } | undefined; await commitBatch( s.core, @@ -3046,7 +3057,7 @@ module(basename(import.meta.filename), function () { observed = { lockDepth: s.lockDepth(), drains: s.drainCount(), - commits: s.commits.length, + sourceReads: s.sourceReads(), }; }, }, @@ -3063,36 +3074,38 @@ module(basename(import.meta.filename), function () { 1, 'after the drain, so the index it reads is current with the bytes', ); + // Not `commits.length`: everything between the hook and the commit — + // the pre-state read, each entry's staging, the compose — leaves that at + // zero, so a hook that had slipped past staging would still satisfy it. + // A source read is the first thing staging does. assert.strictEqual( - observed?.commits, + observed?.sourceReads, 0, - 'and before anything was staged', + 'and before anything had been read to stage from', ); assert.strictEqual(s.commits.length, 1, 'the write then proceeds'); }); - test('a delete carrying a precondition drains first, though it stages nothing', async function (assert) { - // A removal stages no content, so the batch would otherwise skip the - // drain — and a removal is the one verb whose precondition is answered - // entirely from indexed state, with no staged bytes to read instead. An - // undrained index still spells the pre-write validator, so a stale - // `If-Match` would match and the newer file would be removed. + test('a removal runs its precondition inside the lock too', async function (assert) { + // A removal stages nothing, so it takes neither the drain nor a staging + // read — which leaves the lock as the only thing its precondition can + // rest on, and the only thing a test can check it against. let s = stub({ stored: { 'person-1.json': cardFile({ firstName: 'Original' }, PERSON), }, }); - let drainsWhenChecked: number | undefined; + let lockDepthWhenChecked: number | undefined; await commitBatch(s.core, [{ op: 'delete', href: `${REALM}person-1` }], { precondition: async () => { - drainsWhenChecked = s.drainCount(); + lockDepthWhenChecked = s.lockDepth(); }, }); assert.strictEqual( - drainsWhenChecked, + lockDepthWhenChecked, 1, - 'the drain ran before the precondition, though nothing staged', + 'the removal held the write lock when its precondition ran', ); assert.strictEqual(s.commits.length, 1, 'and the removal then proceeds'); }); diff --git a/packages/runtime-common/card-operations/coordinator.ts b/packages/runtime-common/card-operations/coordinator.ts index c621fc0b3a4..c4bccdd94ed 100644 --- a/packages/runtime-common/card-operations/coordinator.ts +++ b/packages/runtime-common/card-operations/coordinator.ts @@ -293,15 +293,7 @@ export async function commitBatch( // because this wait happens with the realm's write lock held — a removal // issued while a bulk import drains would park here holding it, with // every other writer queued behind. - // A batch that stages nothing skips the drain on its own terms — a removal - // names a file and reads the bytes already there, resolving no definition - // — but a precondition reads *indexed* state, and a removal is exactly the - // verb whose precondition would otherwise be answered from an index that - // has not caught up with the bytes it is about to delete. So a caller that - // brought one forces the wait whatever the batch stages. - let readsIndexedState = - entries.some(stagesContent) || opts.precondition !== undefined; - if (opts.waitForIndex !== false && readsIndexedState) { + if (opts.waitForIndex !== false && entries.some(stagesContent)) { await core.drainIndexing(); } // After the drain, so the state a precondition reads is the realm as this diff --git a/packages/runtime-common/card-operations/types.ts b/packages/runtime-common/card-operations/types.ts index 371cb4f3e53..d15dc467f56 100644 --- a/packages/runtime-common/card-operations/types.ts +++ b/packages/runtime-common/card-operations/types.ts @@ -523,6 +523,12 @@ export type OperationErrorCode = // The request named a `baseVersion` the target is no longer at, on an // operation that requires the base to match. | 'version-conflict' + // A conditional request could not be decided, as distinct from being + // decided against: the realm could not establish that the state it would + // compare is current, so it refused rather than answer from state it knows + // may be behind. Carries a 5xx rather than a 412 because nothing about the + // caller's request is wrong and repeating it unchanged is the remedy. + | 'precondition-unverifiable' // The bytes an operation would store are over the realm's ceiling for a // card or a file of that kind. Separate from `invalid-params` because the // payload is well formed and the remedy is to send less of it, and because diff --git a/packages/runtime-common/jobs/indexing.ts b/packages/runtime-common/jobs/indexing.ts index c6c0d00aa13..fc3ade0213c 100644 --- a/packages/runtime-common/jobs/indexing.ts +++ b/packages/runtime-common/jobs/indexing.ts @@ -239,7 +239,7 @@ export async function awaitRealmIndexSettled( `SELECT 1 FROM jobs WHERE status = 'unfulfilled' AND concurrency_group =`, param(indexingConcurrencyGroup(realmURL)), ]; - if (jobTypes) { + if (jobTypes?.length) { expression.push('AND job_type IN', '('); jobTypes.forEach((jobType, index) => { if (index > 0) { diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 312793f7f62..7b9e8909f41 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -4305,30 +4305,7 @@ export class Realm { localPath, ), drainIndexing: async () => { - // Two halves, because neither sees what the other does. The - // in-memory deferreds cover the jobs this process enqueued and - // nothing else — they are a `Map` on this replica's index updater — - // so a write taken by a peer replica, or one deferred to a worker, - // is invisible here. The jobs table is shared, so a query against - // the realm's indexing lane sees every replica's pending work. - // - // Without the second half the gate is only as good as the realm - // having one replica: a `skip-index-wait` write on replica A leaves - // A's index job pending while replica B, seeing nothing local to - // drain, reads a row that still describes the pre-write card. - // - // Scoped to the job types the write path actually races. A - // from-scratch pass reads files independently of realm-server - // writes, so waiting on one would park every write behind a - // system-wide reindex for as long as it takes — the same exclusion - // `incrementalIndexing()` makes in memory, which is why the two are - // defined against one list. await this.incrementalIndexing(); - if (this.#dbAdapter) { - await awaitRealmIndexSettled(this.#dbAdapter, this.url, { - jobTypes: WRITE_RACING_INDEX_JOB_TYPES, - }); - } }, isIgnored: (url) => this.isIgnored(url), // Narrowed to the two documents a program reads values from, each @@ -7830,6 +7807,49 @@ export class Realm { }); }; return async () => { + // The validator is built from the index, and the index lags the bytes: + // a commit records a file's hash before it indexes, and a write that + // deferred its indexing releases the lock without having indexed at all. + // The lock this runs inside stops another writer landing bytes, but says + // nothing about indexing already in flight — and the realm's local view + // of that is an in-memory map of the jobs *this* process enqueued, so a + // peer replica's pending job is invisible to it. The realm's indexing + // lane in the shared jobs table is the view every replica writes to. + // + // Scoped to the job types a write races. A from-scratch pass reads files + // independently of realm-server writes, so waiting on one would hold + // this lock for as long as a system-wide reindex takes. + // + // Refusing when the lane will not settle is the point of checking at + // all: an unsettled lane is exactly the state in which the row still + // describes the pre-write card, so a validator the caller has been + // overtaken by would match. Answering from it would turn the congestion + // this guards against into a silent accept — and silently, since a lane + // that never drains looks from here like a realm with nothing to + // refuse. The refusal is a 5xx rather than a 412 because nothing about + // the caller's request is wrong and repeating it is the remedy. + if (this.#dbAdapter) { + let settled = await awaitRealmIndexSettled(this.#dbAdapter, this.url, { + jobTypes: WRITE_RACING_INDEX_JOB_TYPES, + }); + if (!settled) { + this.#log.warn( + `conditional ${request.method} of ${url.href} refused: ` + + `${indexingConcurrencyGroup(this.url)} did not settle, so the ` + + `index cannot be compared against`, + ); + throw new OperationFailure({ + id: url.href, + status: 503, + code: 'precondition-unverifiable', + title: 'Service Unavailable', + detail: + `the realm could not establish that its index is current for ` + + `${url.href}, so it cannot decide whether the card still ` + + `matches If-Match: ${ifMatch}`, + }); + } + } await this.getRealmInfo(); let entry = await this.#realmIndexQueryEngine.instance(url, { includeErrors: true, @@ -8208,7 +8228,16 @@ export class Realm { ...identity, }); } - throw new CardError(detail, { status, title }); + // Identity travels on every other branch, so it travels on this one too: + // a refusal that names no card is harder to act on than one that does, and + // the 412 a conditional write answers with is precisely a refusal about a + // particular card. + throw new CardError(detail, { + status, + title, + ...(identity.id ? { id: identity.id } : {}), + ...(identity.lid ? { lid: identity.lid } : {}), + }); } // Card+JSON ETags are unsafe when the card has dependencies that live From 3160fb9dee72a835e0386633f89a460d17cdf340 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 04:30:33 -0400 Subject: [PATCH 08/14] Bound the conditional write's wait, and exercise the refusal it ends in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate waits with the realm's write lock held, so every other writer is queued behind it and a long wait spends their latency to answer one request. It takes its own short budget rather than the one a readiness probe uses, where nothing is waiting on the answer. That also makes the refusal reachable from a test: an unfulfilled job parked in the realm's indexing lane is a lane that never drains, without needing a slow index pass to arrange one. Two tests cover it — a conditional write is refused rather than answered from an index the realm cannot vouch for, and an unconditional one is untouched, since a caller that named no validator asked nothing an unsettled index could leave undecidable. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-conditional-write-test.ts | 88 +++++++++++++++++++ packages/runtime-common/realm.ts | 10 +++ 2 files changed, 98 insertions(+) diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts index a7646a66273..27f78ad038a 100644 --- a/packages/realm-server/tests/card-conditional-write-test.ts +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -8,6 +8,7 @@ import fsExtra from 'fs-extra'; const { existsSync, readFileSync } = fsExtra; import type { Realm } from '@cardstack/runtime-common'; import { rri } from '@cardstack/runtime-common'; +import { indexingConcurrencyGroup } from '@cardstack/runtime-common/jobs/indexing'; import { setupPermissionedRealmCached, setupMatrixRoom, @@ -433,6 +434,93 @@ module(basename(import.meta.filename), function () { ); }); + test('a conditional write the realm cannot decide is refused, not answered', async function (assert) { + // The validator is built from the index, so a lane that will not + // settle is exactly the state in which the row still describes the + // pre-write card and a stale validator would match. Parking an + // unfulfilled job in the realm's indexing lane puts the gate in that + // state without needing a real slow index pass: nothing claims it, so + // it never drains. + let read = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let etag = read.get('etag') ?? ''; + assert.ok(etag, 'the read hands out a validator'); + + let inserted = (await dbAdapter.execute( + `INSERT INTO jobs (job_type, concurrency_group, args, status, timeout) + VALUES ($1, $2, '{}'::jsonb, 'unfulfilled', 7200) + RETURNING id`, + { + bind: [ + 'incremental-index', + indexingConcurrencyGroup(realmURL.href), + ], + }, + )) as unknown as { id: string }[]; + + try { + let response = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', etag); + + // Not a 412: the caller's validator may well be current, and the + // realm is saying it cannot tell — which is its own fault to report, + // and which repeating the request fixes. + assert.strictEqual( + response.status, + 503, + `HTTP 503 status: ${response.text}`, + ); + assert.false( + readFileSync(cardFile('person-1.json'), 'utf8').includes( + 'Van Gogh', + ), + 'and the write did not land', + ); + } finally { + await dbAdapter.execute('DELETE FROM jobs WHERE id = $1', { + bind: [inserted[0].id], + }); + } + }); + + test('an unconditional write is unaffected by a lane that will not settle', async function (assert) { + // The gate belongs to the precondition, not to writing: a caller that + // named no validator asked nothing that an unsettled index could make + // undecidable, and must not inherit a refusal from one. + let inserted = (await dbAdapter.execute( + `INSERT INTO jobs (job_type, concurrency_group, args, status, timeout) + VALUES ($1, $2, '{}'::jsonb, 'unfulfilled', 7200) + RETURNING id`, + { + bind: [ + 'incremental-index', + indexingConcurrencyGroup(realmURL.href), + ], + }, + )) as unknown as { id: string }[]; + + try { + let response = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + + assert.strictEqual( + response.status, + 200, + `HTTP 200 status: ${response.text}`, + ); + } finally { + await dbAdapter.execute('DELETE FROM jobs WHERE id = $1', { + bind: [inserted[0].id], + }); + } + }); + test('a write carrying no If-Match is unaffected by any of this', async function (assert) { let response = await request .patch('/person-1') diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 7b9e8909f41..43174fa9a94 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -542,6 +542,15 @@ const READINESS_REQUEST_BUDGET_MS = 10_000; const READ_INDEX_DRAIN_BUDGET_MS = 10_000; const MODULE_ETAG_VARIANT = 'module'; const SOURCE_ETAG_VARIANT = 'source'; +// How long a conditional write waits for the realm's indexing lane before it +// gives up and refuses. Shorter than the budget a readiness probe takes, +// because this one waits with the realm's write lock held: every other writer +// to the realm is queued behind it, so a long wait spends other requests' +// latency to answer this one. A caller whose write is refused this way retries +// — by which time the lane has usually moved — where a caller queued behind a +// ten-second hold has already paid for it. +const CONDITIONAL_WRITE_INDEX_SETTLE_BUDGET_MS = 3_000; + // Card+JSON ETag is `"-[-]:card"` // — quoted per RFC 9110 §8.8.3 so CDNs / browsers don't re-quote inbound // validators and split the cache key. Three inputs feed the base: @@ -7831,6 +7840,7 @@ export class Realm { if (this.#dbAdapter) { let settled = await awaitRealmIndexSettled(this.#dbAdapter, this.url, { jobTypes: WRITE_RACING_INDEX_JOB_TYPES, + timeoutMs: CONDITIONAL_WRITE_INDEX_SETTLE_BUDGET_MS, }); if (!settled) { this.#log.warn( From 08c001c50155ccc834404c91961674a39ca5beeb Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 04:57:57 -0400 Subject: [PATCH 09/14] Wedge the lane the way one actually wedges, not with a job the worker eats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare unfulfilled row is not a lane that will not settle: the worker claims it, fails on the empty args, and poisons the realm's next index job instead. The refusal test passed anyway while its premise was never established, and the damage surfaced as a neighbouring write's 500 — the passing test was the misleading one. A job claimed under a far-future reservation is the condition as it arises: nothing runs it, and the lane stays occupied. The companion test asserting an unconditional write is unaffected is gone rather than repaired. Against a genuinely wedged lane it would block on its own index job, which cannot be claimed while the group is held, so it would have been red for reasons that have nothing to do with the gate it named. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-conditional-write-test.ts | 101 ++++++++---------- 1 file changed, 44 insertions(+), 57 deletions(-) diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts index 27f78ad038a..c39dbe1d78c 100644 --- a/packages/realm-server/tests/card-conditional-write-test.ts +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -434,20 +434,14 @@ module(basename(import.meta.filename), function () { ); }); - test('a conditional write the realm cannot decide is refused, not answered', async function (assert) { - // The validator is built from the index, so a lane that will not - // settle is exactly the state in which the row still describes the - // pre-write card and a stale validator would match. Parking an - // unfulfilled job in the realm's indexing lane puts the gate in that - // state without needing a real slow index pass: nothing claims it, so - // it never drains. - let read = await request - .get('/person-1') - .set('Accept', 'application/vnd.card+json'); - let etag = read.get('etag') ?? ''; - assert.ok(etag, 'the read hands out a validator'); - - let inserted = (await dbAdapter.execute( + // A lane that will not settle, arranged the way one actually arises: a + // job that is claimed and never completes. A bare unfulfilled row is not + // that — the worker picks it up, and with empty args it fails, which + // poisons the realm's next index job rather than leaving the lane stuck. + // Holding a reservation with a far-future lock is the "a worker died + // with a job claimed" case, and nothing runs it. + async function wedgeIndexingLane() { + let job = (await dbAdapter.execute( `INSERT INTO jobs (job_type, concurrency_group, args, status, timeout) VALUES ($1, $2, '{}'::jsonb, 'unfulfilled', 7200) RETURNING id`, @@ -458,7 +452,37 @@ module(basename(import.meta.filename), function () { ], }, )) as unknown as { id: string }[]; + let jobId = job[0].id; + await dbAdapter.execute( + `INSERT INTO job_reservations (job_id, worker_id, locked_until) + VALUES ($1, $2, NOW() + INTERVAL '7200 seconds')`, + { bind: [jobId, 'conditional-write-test-worker'] }, + ); + return async () => { + await dbAdapter.execute( + 'DELETE FROM job_reservations WHERE job_id = $1', + { bind: [jobId] }, + ); + await dbAdapter.execute('DELETE FROM jobs WHERE id = $1', { + bind: [jobId], + }); + }; + } + test('a conditional write the realm cannot decide is refused, not answered', async function (assert) { + // The validator is built from the index, so a lane that will not + // settle is exactly the state in which the row still describes the + // pre-write card and a stale validator would match. Refusing is the + // point of checking at all: answering would turn the congestion this + // guards against into a silent accept. + let read = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + let etag = read.get('etag') ?? ''; + assert.ok(etag, 'the read hands out a validator'); + let bytesBefore = readFileSync(cardFile('person-1.json'), 'utf8'); + + let unwedge = await wedgeIndexingLane(); try { let response = await request .patch('/person-1') @@ -467,57 +491,20 @@ module(basename(import.meta.filename), function () { .set('If-Match', etag); // Not a 412: the caller's validator may well be current, and the - // realm is saying it cannot tell — which is its own fault to report, - // and which repeating the request fixes. + // realm is saying it cannot tell — its own fault to report, and one + // that repeating the request fixes. assert.strictEqual( response.status, 503, `HTTP 503 status: ${response.text}`, ); - assert.false( - readFileSync(cardFile('person-1.json'), 'utf8').includes( - 'Van Gogh', - ), - 'and the write did not land', - ); - } finally { - await dbAdapter.execute('DELETE FROM jobs WHERE id = $1', { - bind: [inserted[0].id], - }); - } - }); - - test('an unconditional write is unaffected by a lane that will not settle', async function (assert) { - // The gate belongs to the precondition, not to writing: a caller that - // named no validator asked nothing that an unsettled index could make - // undecidable, and must not inherit a refusal from one. - let inserted = (await dbAdapter.execute( - `INSERT INTO jobs (job_type, concurrency_group, args, status, timeout) - VALUES ($1, $2, '{}'::jsonb, 'unfulfilled', 7200) - RETURNING id`, - { - bind: [ - 'incremental-index', - indexingConcurrencyGroup(realmURL.href), - ], - }, - )) as unknown as { id: string }[]; - - try { - let response = await request - .patch('/person-1') - .send(patchPersonBody('Van Gogh')) - .set('Accept', 'application/vnd.card+json'); - assert.strictEqual( - response.status, - 200, - `HTTP 200 status: ${response.text}`, + readFileSync(cardFile('person-1.json'), 'utf8'), + bytesBefore, + 'and the stored file is untouched', ); } finally { - await dbAdapter.execute('DELETE FROM jobs WHERE id = $1', { - bind: [inserted[0].id], - }); + await unwedge(); } }); From d654a1ad7ba0dd9c8c9f6cfa026f1fed80946716 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 05:13:14 -0400 Subject: [PATCH 10/14] Ask the whole indexing lane, not the job types a write races The scoping was inherited from the drain, which excludes a from-scratch pass because it decides whether to wait, and waiting on a system-wide reindex would park every writer behind it. This decides whether the index can be believed, and a from-scratch pass is among the strongest reasons it cannot: a realm republish swaps the files under the write lock and enqueues one before releasing, so the row still describes the pre-swap card while the bytes are already the new ones. A conditional write arriving then read a validator the realm had already moved past and matched it. Asking about the lane itself also cannot fall out of step with a job type added later, and takes the option and its constant back out. Co-Authored-By: Claude Opus 5 (1M context) --- packages/runtime-common/jobs/indexing.ts | 43 +++--------------------- packages/runtime-common/realm.ts | 14 +++++--- 2 files changed, 14 insertions(+), 43 deletions(-) diff --git a/packages/runtime-common/jobs/indexing.ts b/packages/runtime-common/jobs/indexing.ts index fc3ade0213c..86e0ff9c9a2 100644 --- a/packages/runtime-common/jobs/indexing.ts +++ b/packages/runtime-common/jobs/indexing.ts @@ -6,12 +6,7 @@ import type { IncrementalDoneResult, IncrementalResult, } from '../tasks/indexer.ts'; -import { - param, - query, - type Expression, - type PgPrimitive, -} from '../expression.ts'; +import { param, query, type PgPrimitive } from '../expression.ts'; import type { DBAdapter } from '../db.ts'; import { baseRealm, baseRealmRRI } from '../constants.ts'; import { systemInitiatedPriority, userInitiatedPriority } from '../queue.ts'; @@ -205,26 +200,10 @@ export async function unbuiltIndexFailure( return typeof result === 'string' ? result : JSON.stringify(result); } -// The job types a realm's write path races against: the two that rewrite index -// rows for files someone just wrote. `from-scratch-index` is deliberately not -// among them — it reads files independently of realm-server writes and each row -// write is atomic, so blocking a `PATCH` on one would park user writes behind a -// system-wide reindex for as long as that takes. This is the cross-replica -// spelling of the scope `RealmIndexUpdater.incrementalIndexing()` keeps in -// memory, and the two must stay in step. -export const WRITE_RACING_INDEX_JOB_TYPES = ['incremental-index', 'copy-index']; - export async function awaitRealmIndexSettled( dbAdapter: DBAdapter, realmURL: string, - opts?: { - timeoutMs?: number; - pollIntervalMs?: number; - // Narrows the lane to particular job types. Absent means the whole - // `indexing:` lane, which is what a caller wanting "all indexing has - // settled" (a publish, a readiness probe) asks for. - jobTypes?: string[]; - }, + opts?: { timeoutMs?: number; pollIntervalMs?: number }, ): Promise { if (dbAdapter.kind !== 'pg') { return true; @@ -232,25 +211,13 @@ export async function awaitRealmIndexSettled( let timeoutMs = opts?.timeoutMs ?? 10_000; let pollIntervalMs = opts?.pollIntervalMs ?? 1000; - let jobTypes = opts?.jobTypes; let hasSettled = async () => { - let expression: Expression = [ + let rows = await query(dbAdapter, [ `SELECT 1 FROM jobs WHERE status = 'unfulfilled' AND concurrency_group =`, param(indexingConcurrencyGroup(realmURL)), - ]; - if (jobTypes?.length) { - expression.push('AND job_type IN', '('); - jobTypes.forEach((jobType, index) => { - if (index > 0) { - expression.push(','); - } - expression.push(param(jobType)); - }); - expression.push(')'); - } - expression.push('LIMIT 1'); - let rows = await query(dbAdapter, expression); + 'LIMIT 1', + ]); return rows.length === 0; }; diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 43174fa9a94..2c1181fe85f 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -6,7 +6,6 @@ import { INCREMENTAL_INDEX_JOB_TIMEOUT_SEC, prerenderSpawnedPriority, unbuiltIndexFailure, - WRITE_RACING_INDEX_JOB_TYPES, } from './jobs/indexing.ts'; import { awaitPublishedHtmlReady, @@ -7825,9 +7824,15 @@ export class Realm { // peer replica's pending job is invisible to it. The realm's indexing // lane in the shared jobs table is the view every replica writes to. // - // Scoped to the job types a write races. A from-scratch pass reads files - // independently of realm-server writes, so waiting on one would hold - // this lock for as long as a system-wide reindex takes. + // The whole lane, not the job types a write races. The drain excludes a + // from-scratch pass because it decides whether to *wait*, and waiting on + // a system-wide reindex would park every writer behind it. This decides + // whether the index can be *believed*, and a from-scratch pass is one of + // the strongest reasons it cannot: a realm republish swaps the files + // under the write lock and enqueues one before releasing, so the row + // still describes the pre-swap card while the bytes are already the new + // ones. Any job in the lane means the same thing here — that something + // is on its way to changing what the index says. // // Refusing when the lane will not settle is the point of checking at // all: an unsettled lane is exactly the state in which the row still @@ -7839,7 +7844,6 @@ export class Realm { // the caller's request is wrong and repeating it is the remedy. if (this.#dbAdapter) { let settled = await awaitRealmIndexSettled(this.#dbAdapter, this.url, { - jobTypes: WRITE_RACING_INDEX_JOB_TYPES, timeoutMs: CONDITIONAL_WRITE_INDEX_SETTLE_BUDGET_MS, }); if (!settled) { From 3af0cbbaea5044dec06dcd2d8121fc744efcf597 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 07:46:22 -0400 Subject: [PATCH 11/14] Wait only on the jobs that move bytes, and bound the wait to the lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asking whether the realm's indexing lane was occupied at all refused conditional writes for the length of anything in it. A from-scratch pass re-derives rows from files nobody changed — and one lands in every realm's lane after a deploy that moves the UI checksum — so writes would have been refused fleet-wide for an hour to guard a content change that never happened. A daily stylesheet GC shares the lane and moves no row at all. The scope is now derived from what moves a card's bytes rather than inherited: only a write does, and a write's indexing is incremental or copy. The republish case that argued for including a from-scratch pass cannot reach a conditional write — a published realm grants read to everyone and write to nobody. The budget drops to a second with a matching poll, because the wait is held under the realm write lock and each poll takes a pool client while that lock already pins one. The removal's placement test now also pins that nothing was committed when its precondition ran, and the wedge fixture cleans up a half-built wedge rather than leaving behind the bare job it exists to avoid. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-conditional-write-test.ts | 39 +++++++++++-- .../tests/card-operations-batch-test.ts | 15 ++++- .../runtime-common/card-operations/types.ts | 6 ++ packages/runtime-common/jobs/indexing.ts | 56 +++++++++++++++++-- packages/runtime-common/realm.ts | 44 +++++++++------ 5 files changed, 130 insertions(+), 30 deletions(-) diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts index c39dbe1d78c..de0a4754043 100644 --- a/packages/realm-server/tests/card-conditional-write-test.ts +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -453,12 +453,7 @@ module(basename(import.meta.filename), function () { }, )) as unknown as { id: string }[]; let jobId = job[0].id; - await dbAdapter.execute( - `INSERT INTO job_reservations (job_id, worker_id, locked_until) - VALUES ($1, $2, NOW() + INTERVAL '7200 seconds')`, - { bind: [jobId, 'conditional-write-test-worker'] }, - ); - return async () => { + let unwedge = async () => { await dbAdapter.execute( 'DELETE FROM job_reservations WHERE job_id = $1', { bind: [jobId] }, @@ -467,6 +462,23 @@ module(basename(import.meta.filename), function () { bind: [jobId], }); }; + try { + // The reservation is what stops a worker claiming the job. Until it + // lands, what is in the lane is the bare row this fixture exists to + // avoid — a job the worker picks up and dies on, poisoning the + // realm's next index pass. A throw here would leave that behind with + // nothing to remove it, and every later conditional write in this + // file would wait out its budget and 503. + await dbAdapter.execute( + `INSERT INTO job_reservations (job_id, worker_id, locked_until) + VALUES ($1, $2, NOW() + INTERVAL '7200 seconds')`, + { bind: [jobId, 'conditional-write-test-worker'] }, + ); + } catch (err) { + await unwedge(); + throw err; + } + return unwedge; } test('a conditional write the realm cannot decide is refused, not answered', async function (assert) { @@ -506,6 +518,21 @@ module(basename(import.meta.filename), function () { } finally { await unwedge(); } + + // The control: the same request, unchanged, against an unwedged lane. + // Without it a 503 from any other cause would read as this one's, and + // the test would pass on a realm that refuses conditional writes for + // reasons that have nothing to do with the wedge. + let afterUnwedge = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', etag); + assert.strictEqual( + afterUnwedge.status, + 200, + `the same write succeeds once the lane clears: ${afterUnwedge.text}`, + ); }); test('a write carrying no If-Match is unaffected by any of this', async function (assert) { diff --git a/packages/realm-server/tests/card-operations-batch-test.ts b/packages/realm-server/tests/card-operations-batch-test.ts index 25f79b1b6b0..86ac2251402 100644 --- a/packages/realm-server/tests/card-operations-batch-test.ts +++ b/packages/realm-server/tests/card-operations-batch-test.ts @@ -3095,18 +3095,27 @@ module(basename(import.meta.filename), function () { 'person-1.json': cardFile({ firstName: 'Original' }, PERSON), }, }); - let lockDepthWhenChecked: number | undefined; + let observed: { lockDepth: number; commits: number } | undefined; await commitBatch(s.core, [{ op: 'delete', href: `${REALM}person-1` }], { precondition: async () => { - lockDepthWhenChecked = s.lockDepth(); + observed = { lockDepth: s.lockDepth(), commits: s.commits.length }; }, }); assert.strictEqual( - lockDepthWhenChecked, + observed?.lockDepth, 1, 'the removal held the write lock when its precondition ran', ); + // A removal reads nothing to stage from, so the commit count is the only + // thing that places it ahead of the removal itself. Without this the + // test is satisfied by a hook anywhere inside the lock, including after + // the file is gone. + assert.strictEqual( + observed?.commits, + 0, + 'and before the removal was committed', + ); assert.strictEqual(s.commits.length, 1, 'and the removal then proceeds'); }); diff --git a/packages/runtime-common/card-operations/types.ts b/packages/runtime-common/card-operations/types.ts index d15dc467f56..d27caac6d73 100644 --- a/packages/runtime-common/card-operations/types.ts +++ b/packages/runtime-common/card-operations/types.ts @@ -528,6 +528,12 @@ export type OperationErrorCode = // compare is current, so it refused rather than answer from state it knows // may be behind. Carries a 5xx rather than a 412 because nothing about the // caller's request is wrong and repeating it unchanged is the remedy. + // + // Internal taxonomy on the card verbs. Those refusals reach a client as a + // status and a sentence — `#cardWriteRefusal` carries the status and the + // detail, not this — so a caller there tells this from any other 5xx by + // what the detail says. It is on the wire only where an operation result + // carries its own error, which is the envelope. | 'precondition-unverifiable' // The bytes an operation would store are over the realm's ceiling for a // card or a file of that kind. Separate from `invalid-params` because the diff --git a/packages/runtime-common/jobs/indexing.ts b/packages/runtime-common/jobs/indexing.ts index 86e0ff9c9a2..08aefc6c6e0 100644 --- a/packages/runtime-common/jobs/indexing.ts +++ b/packages/runtime-common/jobs/indexing.ts @@ -6,7 +6,12 @@ import type { IncrementalDoneResult, IncrementalResult, } from '../tasks/indexer.ts'; -import { param, query, type PgPrimitive } from '../expression.ts'; +import { + param, + query, + type Expression, + type PgPrimitive, +} from '../expression.ts'; import type { DBAdapter } from '../db.ts'; import { baseRealm, baseRealmRRI } from '../constants.ts'; import { systemInitiatedPriority, userInitiatedPriority } from '../queue.ts'; @@ -200,10 +205,38 @@ export async function unbuiltIndexFailure( return typeof result === 'string' ? result : JSON.stringify(result); } +// The indexing jobs that can leave a card's index row describing bytes the +// realm no longer stores — which is the only thing a reader comparing an +// index-derived validator against the stored file needs to wait for. +// +// Derived from what moves BYTES, not from what the write-path drain happens to +// wait for. Only a write moves a card's stored file, and a write's indexing is +// one of these two. A `from-scratch-index` re-derives rows from files nobody +// changed, so it moves `indexed_at` without moving content — and the one case +// where it follows a real content change, a realm republish, cannot matter +// here: a published realm is created with `['read', 'realm-owner']` and +// `'*': ['read']` and grants write to nobody, so no conditional write can +// reach one. `scoped-css-gc` shares the lane too and only deletes unreferenced +// stylesheet rows. +// +// Narrow on purpose: this list decides who WAITS, and the lane is shared with +// passes that run fleet-wide for an hour at a time. +export const CONTENT_MOVING_INDEX_JOB_TYPES = [ + 'incremental-index', + 'copy-index', +]; + export async function awaitRealmIndexSettled( dbAdapter: DBAdapter, realmURL: string, - opts?: { timeoutMs?: number; pollIntervalMs?: number }, + opts?: { + timeoutMs?: number; + pollIntervalMs?: number; + // Narrows the lane to particular job types. Absent means the whole + // `indexing:` lane, which is what a caller wanting "all indexing + // has settled" — a readiness probe, a publish — asks for. + jobTypes?: string[]; + }, ): Promise { if (dbAdapter.kind !== 'pg') { return true; @@ -212,12 +245,25 @@ export async function awaitRealmIndexSettled( let timeoutMs = opts?.timeoutMs ?? 10_000; let pollIntervalMs = opts?.pollIntervalMs ?? 1000; + let jobTypes = opts?.jobTypes; + let hasSettled = async () => { - let rows = await query(dbAdapter, [ + let expression: Expression = [ `SELECT 1 FROM jobs WHERE status = 'unfulfilled' AND concurrency_group =`, param(indexingConcurrencyGroup(realmURL)), - 'LIMIT 1', - ]); + ]; + if (jobTypes?.length) { + expression.push('AND job_type IN', '('); + jobTypes.forEach((jobType, index) => { + if (index > 0) { + expression.push(','); + } + expression.push(param(jobType)); + }); + expression.push(')'); + } + expression.push('LIMIT 1'); + let rows = await query(dbAdapter, expression); return rows.length === 0; }; diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 2c1181fe85f..a11c2439984 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -4,6 +4,7 @@ import { awaitRealmIndexSettled, indexingConcurrencyGroup, INCREMENTAL_INDEX_JOB_TIMEOUT_SEC, + CONTENT_MOVING_INDEX_JOB_TYPES, prerenderSpawnedPriority, unbuiltIndexFailure, } from './jobs/indexing.ts'; @@ -542,13 +543,22 @@ const READ_INDEX_DRAIN_BUDGET_MS = 10_000; const MODULE_ETAG_VARIANT = 'module'; const SOURCE_ETAG_VARIANT = 'source'; // How long a conditional write waits for the realm's indexing lane before it -// gives up and refuses. Shorter than the budget a readiness probe takes, -// because this one waits with the realm's write lock held: every other writer -// to the realm is queued behind it, so a long wait spends other requests' -// latency to answer this one. A caller whose write is refused this way retries -// — by which time the lane has usually moved — where a caller queued behind a -// ten-second hold has already paid for it. -const CONDITIONAL_WRITE_INDEX_SETTLE_BUDGET_MS = 3_000; +// gives up and refuses, and how often it re-asks while waiting. +// +// Much shorter than the budget a readiness probe takes, because this one waits +// with the realm's write lock held and each poll takes a pool client while the +// lock already pins one. Every other writer to the realm is queued behind it, +// so the wait spends their latency to answer this request — and the 503 it +// ends in invites a retry that takes the lock again. +// +// A long budget buys little anyway: the jobs it waits on are bounded in +// minutes, so anything that would finish inside a wait of this size was +// finishing regardless, and the `jobs_finished` subscription delivers the +// wakeup rather than the poll. The poll is the backstop, so it is sized to the +// budget rather than left at a default that would fire only a few times inside +// it. +const CONDITIONAL_WRITE_INDEX_SETTLE_BUDGET_MS = 1_000; +const CONDITIONAL_WRITE_INDEX_SETTLE_POLL_MS = 250; // Card+JSON ETag is `"-[-]:card"` // — quoted per RFC 9110 §8.8.3 so CDNs / browsers don't re-quote inbound @@ -7824,15 +7834,15 @@ export class Realm { // peer replica's pending job is invisible to it. The realm's indexing // lane in the shared jobs table is the view every replica writes to. // - // The whole lane, not the job types a write races. The drain excludes a - // from-scratch pass because it decides whether to *wait*, and waiting on - // a system-wide reindex would park every writer behind it. This decides - // whether the index can be *believed*, and a from-scratch pass is one of - // the strongest reasons it cannot: a realm republish swaps the files - // under the write lock and enqueues one before releasing, so the row - // still describes the pre-swap card while the bytes are already the new - // ones. Any job in the lane means the same thing here — that something - // is on its way to changing what the index says. + // Scoped to the jobs that can leave a row describing bytes the realm no + // longer stores, which is the only staleness a validator comparison can + // be wrong about. Asking whether the lane is occupied at all would be + // fail-closed and wrong in practice: a from-scratch pass re-derives rows + // from files nobody changed, and one lands in every realm's lane after + // any deploy that moves the UI checksum, so conditional writes would be + // refused fleet-wide for the length of a reindex to guard against a + // content change that did not happen. A daily stylesheet GC shares the + // lane too and moves no row at all. // // Refusing when the lane will not settle is the point of checking at // all: an unsettled lane is exactly the state in which the row still @@ -7844,7 +7854,9 @@ export class Realm { // the caller's request is wrong and repeating it is the remedy. if (this.#dbAdapter) { let settled = await awaitRealmIndexSettled(this.#dbAdapter, this.url, { + jobTypes: CONTENT_MOVING_INDEX_JOB_TYPES, timeoutMs: CONDITIONAL_WRITE_INDEX_SETTLE_BUDGET_MS, + pollIntervalMs: CONDITIONAL_WRITE_INDEX_SETTLE_POLL_MS, }); if (!settled) { this.#log.warn( From fdfc2aef72b64f928dca0755152932b167b1c0c6 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 07:53:31 -0400 Subject: [PATCH 12/14] Compare against every validator shape, enumerated rather than listed The card+json validator now has three shapes, and the conditional write built two of them. A client whose last read was a write echo holds the third, so its validator matched nothing and the write was refused for a reason it could not see or act on. The shapes are declared as values as well as a union and the comparison maps over them, so the next one is covered the day it ships rather than the day someone notices. Co-Authored-By: Claude Opus 5 (1M context) --- packages/runtime-common/realm.ts | 39 ++++++++++++++++---------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index d395e18a2e9..b5918a43cbe 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -922,7 +922,13 @@ function buildEtag( // `skipQueryBackedExpansion` into its own key rather than into the ETag. // Adding a member here is the wrong move for a variation the validator does // not have to carry. -type CardJsonShape = 'full' | 'links-only' | 'write-echo'; +// Listed as values as well as a union, so a reader that must cover every +// shape can enumerate them rather than restate the list. A conditional write +// is such a reader: it compares a caller's validator against every one the +// realm could have issued for the card, and a shape it does not know about is +// a validator it would refuse for no reason the caller can see. +const CARD_JSON_SHAPES = ['full', 'links-only', 'write-echo'] as const; +type CardJsonShape = (typeof CARD_JSON_SHAPES)[number]; function buildCardJsonEtag( indexedAt: number | null | undefined, @@ -8270,27 +8276,20 @@ export class Realm { if (entry?.type !== 'instance' || this.hasForeignRealmDeps(entry.deps)) { refuse(); } - // Every validator the realm would hand out for this card as it stands. - // A card+json read picks its link shape per request — the setting can - // differ between the read that gave the client its validator and this - // write — and the shapes take different variants of one validator so a - // client holding either is not 304'd to the other. That distinction is - // about representations; this question is about the card, and all of - // these describe the same card at the same `indexed_at`. Refusing a - // write because the read that preceded it answered in the narrower - // shape would refuse on a server setting rather than on anything the - // caller did. + // Every validator the realm would hand out for this card as it stands, + // enumerated rather than listed, so a shape added later is covered here + // the day it ships. The shapes exist so a client holding one + // representation is not 304'd to another, which makes them a fact about + // representations — and this question is about the card. All of them + // describe the same card at the same `indexed_at`, so refusing over + // which shape a preceding read happened to answer in would refuse on a + // server setting, or on whether the caller's last read was a write + // echo, rather than on anything the caller did. let realmInfoHash = this.getCachedRealmInfoHash(); let screenshots = screenshotsEtagFingerprint(entry!.screenshots); - let issued = [ - buildCardJsonEtag(entry!.indexedAt, realmInfoHash, screenshots, 'full'), - buildCardJsonEtag( - entry!.indexedAt, - realmInfoHash, - screenshots, - 'links-only', - ), - ]; + let issued = CARD_JSON_SHAPES.map((shape) => + buildCardJsonEtag(entry!.indexedAt, realmInfoHash, screenshots, shape), + ); if (!issued.some((etag) => etag && ifNoneMatchMatches(ifMatch, etag))) { refuse(); } From 10252cb3f5d85f00cd9daca0ea3eacf18cd23f97 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 10:53:31 -0400 Subject: [PATCH 13/14] Exercise the validator a write hands back, and say what the enumeration misses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every validator in the conditional-write suite came from a read, so the shape a write response carries — the one a client doing consecutive conditional edits holds, its own last write being the last thing it read — was the only shape with no test behind it, and the newest one in the comparison. The comparison's claim is narrowed to match what it does. Enumerating the validator builder's arguments covers another enumerable argument; it does not cover a value folded into a variant, and the bounded shape folds the assembled-resource budget. That is uniform across a deployment today, so it is reachable only across a rolling deploy that retunes it — and if the budget ever varies per request, the number has to leave the validator. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/card-conditional-write-test.ts | 36 +++++++++++++++++++ packages/runtime-common/realm.ts | 20 ++++++++--- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/packages/realm-server/tests/card-conditional-write-test.ts b/packages/realm-server/tests/card-conditional-write-test.ts index 0ed1cae1512..9ba864a407e 100644 --- a/packages/realm-server/tests/card-conditional-write-test.ts +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -258,6 +258,42 @@ module(basename(import.meta.filename), function () { ); }); + test('a validator a write handed back is accepted by the next write', async function (assert) { + // The validator a client doing consecutive conditional edits holds is + // the one its own last write returned, not one from a read — and a + // write response spells it in its own shape. Every other validator in + // this file comes from a `GET`, so without this the gate's coverage of + // that shape rests on reading the comparison rather than on a test. + let written = await request + .patch('/person-1') + .send(patchPersonBody('Van Gogh')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + written.status, + 200, + `HTTP 200 status: ${written.text}`, + ); + let echoEtag = written.get('etag') ?? ''; + assert.ok(echoEtag, 'the write hands back a validator'); + + let response = await request + .patch('/person-1') + .send(patchPersonBody('Paper')) + .set('Accept', 'application/vnd.card+json') + .set('If-Match', echoEtag); + + assert.strictEqual( + response.status, + 200, + `the echo's own validator is accepted: ${response.text}`, + ); + assert.strictEqual( + response.body?.data?.attributes?.firstName, + 'Paper', + 'and the second edit lands', + ); + }); + test('a refused write enqueues no indexing and broadcasts no event', async function (assert) { let read = await request .get('/person-1') diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 26a5eed3eff..9b6e193def4 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -8326,11 +8326,21 @@ export class Realm { refuse(); } // Every validator the realm would hand out for this card as it stands, - // built by enumerating what the validator builder takes rather than - // what it returns — the shape, and the assembly-budget split the `full` - // shape carries — so a dimension added there is covered here the day it - // ships. Combinations that collapse to one validator are deduplicated - // by the set rather than reasoned about. The shapes exist so a client holding one + // built by enumerating the builder's own arguments rather than the + // spellings they produce — the shape, and the assembly-budget split the + // `full` shape carries. Combinations that collapse to one validator are + // deduplicated by the set rather than reasoned about, so a further + // *enumerable* argument is covered here by adding it to the product. + // + // It does not cover a *value* folded into a variant, and one is: the + // bounded `full` spelling interpolates the assembled-resource budget, so + // only the budget this process is running with is built. That is uniform + // across a deployment today — the value is read at module load — so it + // bites only across a rolling deploy that retunes it, where a validator + // issued by an old replica is refused by a new one. If that budget ever + // varies per request, the number has to come out of the validator, or + // every conditional write starts deciding partly on a server setting, + // which is the failure enumerating the arguments is here to avoid. The shapes exist so a client holding one // representation is not 304'd to another, which makes them a fact about // representations — and this question is about the card. All of them // describe the same card at the same `indexed_at`, so refusing over From 824d02fd70f105d1f6148dfd50c5f5f40c563c19 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Thu, 17 Sep 2026 11:21:40 -0400 Subject: [PATCH 14/14] Say why the validator enumeration cannot be made automatic A new argument to the validator builder arrives optional with a default, so it does not break an existing call and there is no signature change for a type check to catch. Worth recording, so the next reader does not take the absence of a guard for an oversight and go looking for one. Co-Authored-By: Claude Opus 5 (1M context) --- packages/runtime-common/realm.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 890e9dc0c35..2d306ae5941 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -8645,6 +8645,12 @@ export class Realm { // deduplicated by the set rather than reasoned about, so a further // *enumerable* argument is covered here by adding it to the product. // + // Nothing makes that automatic, and it is worth knowing why rather than + // assuming a guard is missing: a new argument here arrives optional with + // a default, which by construction does not break an existing call, so + // there is no signature change for a type check to catch. The product + // below is the only thing that knows this list has to grow. + // // It does not cover a *value* folded into a variant, and one is: the // bounded `full` spelling interpolates the assembled-resource budget, so // only the budget this process is running with is built. That is uniform