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..9ba864a407e --- /dev/null +++ b/packages/realm-server/tests/card-conditional-write-test.ts @@ -0,0 +1,637 @@ +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 { rri } from '@cardstack/runtime-common'; +import { indexingConcurrencyGroup } from '@cardstack/runtime-common/jobs/indexing'; +import { LinkShapePolicy } from '@cardstack/runtime-common/link-shape-policy'; +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'; + +// 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. +// +// 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/'); + 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); + } + + 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' }, + }, + }, + }; + } + + // 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 === indexType, + ); + } + + 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 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') + .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') + .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 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-2') + .send(patchPersonBody('Paper')) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(accepted.status, 200, 'the control write succeeds'); + await waitForIncrementalIndexEvent(getMessagesSince, since); + + let messages = await getMessagesSince(since); + // 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 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.some((urls) => urls.includes('person-2')), + `the control's ${indexType} event arrived in the window`, + ); + assert.false( + named.some((urls) => urls.includes('person-1')), + `no ${indexType} event in the window names the refused card`, + ); + } + }); + + 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', + ); + }); + + // 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`, + { + bind: [ + 'incremental-index', + indexingConcurrencyGroup(realmURL.href), + ], + }, + )) as unknown as { id: string }[]; + let jobId = job[0].id; + let unwedge = 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], + }); + }; + 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) { + // 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') + .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 — its own fault to report, and one + // that repeating the request fixes. + assert.strictEqual( + response.status, + 503, + `HTTP 503 status: ${response.text}`, + ); + assert.strictEqual( + readFileSync(cardFile('person-1.json'), 'utf8'), + bytesBefore, + 'and the stored file is untouched', + ); + } 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) { + 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'); + }); + }); + + // 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'], + }, + linkShapePolicy: LinkShapePolicy.pinned('links-only'), + 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-operations-batch-test.ts b/packages/realm-server/tests/card-operations-batch-test.ts index 7899068353f..5931f0510fd 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, @@ -60,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; // The files the batch asked to be serialized over, sorted. What the lock // covers is not visible in any response, so a test that cares which writers // a batch excludes has to read it here. @@ -120,6 +127,7 @@ function stub(opts: StubOptions = {}): Stub { let maxHeld = 0; let drains = 0; let readsOutsideLock = 0; + let sourceReads = 0; let lockedPaths: string[] = []; let core: BatchCore = { @@ -136,10 +144,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 @@ -147,6 +157,7 @@ function stub(opts: StubOptions = {}): Stub { }, async openSourceBytes(localPath) { readsOutsideLock += held > 0 ? 0 : 1; + sourceReads++; let content = stored[localPath]; if (content === undefined) { return undefined; @@ -249,6 +260,7 @@ function stub(opts: StubOptions = {}): Stub { lockDepth: () => maxHeld, drainCount: () => drains, readsOutsideLock: () => readsOutsideLock, + sourceReads: () => sourceReads, lockedPaths: () => lockedPaths, }; } @@ -3089,5 +3101,154 @@ 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; sourceReads: 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(), + sourceReads: s.sourceReads(), + }; + }, + }, + ); + + assert.ok(observed, 'the precondition was called'); + assert.strictEqual( + observed?.lockDepth, + 1, + "with the batch's file locks held, so what it reads cannot move under it", + ); + assert.strictEqual( + observed?.drains, + 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?.sourceReads, + 0, + 'and before anything had been read to stage from', + ); + assert.strictEqual(s.commits.length, 1, 'the write then proceeds'); + }); + + 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 observed: { lockDepth: number; commits: number } | undefined; + await commitBatch(s.core, [{ op: 'delete', href: `${REALM}person-1` }], { + precondition: async () => { + observed = { lockDepth: s.lockDepth(), commits: s.commits.length }; + }, + }); + + assert.strictEqual( + 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'); + }); + + 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 2a5ead65a35..d7340ad0ed8 100644 --- a/packages/runtime-common/card-operations/coordinator.ts +++ b/packages/runtime-common/card-operations/coordinator.ts @@ -212,6 +212,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 @@ -322,6 +334,10 @@ export async function commitBatch( if (opts.waitForIndex !== false && entries.some(stagesContent)) { await timed('drain', () => 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 locks it already holds. + await opts.precondition?.(); let staged: StagedChange[] = []; // The version each entry's merge was computed over, captured as it stages // rather than read back at the end: `stored` moves underneath the batch diff --git a/packages/runtime-common/card-operations/types.ts b/packages/runtime-common/card-operations/types.ts index 9a983d3b446..9022fbc8af5 100644 --- a/packages/runtime-common/card-operations/types.ts +++ b/packages/runtime-common/card-operations/types.ts @@ -535,6 +535,18 @@ 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. + // + // 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 operation reads the invoking actor and the request authenticated // nobody. Distinct from `invalid-params` because nothing the caller sent is // wrong: the remedy is credentials, which is what its 401 says. 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 497d549c2ab..8ad29f90cfe 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'; @@ -618,6 +619,28 @@ 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, and how often it re-asks while waiting. +// +// Much shorter than the budget a readiness probe takes, because this one waits +// with the batch's file locks held and each poll takes a pool client while the +// lock already pins one. Every other writer of those files 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 locks again. +// +// The scope is narrower than the wait: the locks cover the files this batch +// names, but what it waits for is the realm's indexing lane, so a write parks +// here for work that need not touch its files at all. +// +// 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 // validators and split the cache key. Three inputs feed the base: @@ -941,7 +964,17 @@ function buildEtag( // and the response cache separates it by folding `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. Note that +// covering the shapes is not on its own covering the validators: the `full` +// split above is a second dimension, so a reader enumerates the arguments +// `buildCardJsonEtag` takes rather than this list alone. +const CARD_JSON_SHAPES = ['full', 'links-only', 'write-echo'] as const; +type CardJsonShape = (typeof CARD_JSON_SHAPES)[number]; function buildCardJsonEtag( indexedAt: number | null | undefined, @@ -8544,6 +8577,181 @@ export class Realm { }); } + // 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 locks the write takes — a check made before + // them can pass and then queue behind another writer's whole write of the + // same card, 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 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 running inside the lock, after the drain, is what makes it mean + // anything. + // + // That breadth is also where the guarantee stops, because the lock is + // narrower than the validator. A write locks the files it names, so no + // other writer can move this card's bytes between the check and the commit + // — which is the lost update the header exists to prevent. A linked card's + // re-index moves this validator without touching this file, and that write + // takes a different lock, so a conditional write can still commit against a + // validator that went stale while it ran. That is the trade rather than a + // hole: what is on offer is that this card is still the one the caller + // edited, not that everything its document is assembled from is unchanged. + // A caller that needs the second has no validator to ask for it with, since + // the served document is the only thing either fingerprint describes. + // + // 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. + #conditionalWrite( + request: Request, + url: URL, + ): (() => Promise) | undefined { + let ifMatch = request.headers.get('if-match'); + if (!ifMatch || ifMatch.trim() === '*') { + return undefined; + } + 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 () => { + // 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 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 + // 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: CONTENT_MOVING_INDEX_JOB_TYPES, + timeoutMs: CONDITIONAL_WRITE_INDEX_SETTLE_BUDGET_MS, + pollIntervalMs: CONDITIONAL_WRITE_INDEX_SETTLE_POLL_MS, + }); + 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, + }); + if (entry?.type !== 'instance' || this.hasForeignRealmDeps(entry.deps)) { + refuse(); + } + // Every validator the realm would hand out for this card as it stands, + // 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. + // + // 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 + // 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 + // 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 = new Set( + CARD_JSON_SHAPES.flatMap((shape) => + [false, true].map((unboundedAssembly) => + buildCardJsonEtag( + entry!.indexedAt, + realmInfoHash, + screenshots, + shape, + unboundedAssembly, + ), + ), + ), + ); + if ( + ![...issued].some((etag) => etag && ifNoneMatchMatches(ifMatch, etag)) + ) { + refuse(); + } + }; + } + private async patchCardInstance( request: Request, requestContext: RequestContext, @@ -8605,6 +8813,11 @@ export class Realm { } } + // 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 // echo must never persist, a type that cannot change, the relationship @@ -8650,6 +8863,7 @@ export class Realm { // realm: the edge is stored empty and the patch succeeds. foreignSideLoadLink: 'leave', timings, + ...(precondition ? { precondition } : {}), }, ) )[0]; @@ -8922,7 +9136,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 @@ -9953,6 +10176,7 @@ export class Realm { if (await this.openFileForMetadata(localPath)) { return methodNotAllowed(request, requestContext); } + 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 @@ -9963,6 +10187,7 @@ export class Realm { ...(requestContext.authenticatedUser ? { actor: requestContext.authenticatedUser } : {}), + ...(precondition ? { precondition } : {}), }); } catch (err: unknown) { return this.#cardWriteRefusal(err, request, requestContext, {