Skip to content

Skip the link-closure readback on card writes #6162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lukemelia wants to merge 2 commits into from
+142 −77
Closed

Skip the link-closure readback on card writes #6162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea81b3d
Skip the link-closure readback on card writes
Sep 16, 2026
7a7a0bc
Drop the ETag from card write-echo responses
Sep 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 109 additions & 25 deletions packages/realm-server/tests/card-endpoints-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1815,6 +1815,80 @@ module(basename(import.meta.filename), function () {
);
});

test('the write response omits the transitive link closure the client discards', async function (assert) {
// A create's response is read only for the primary card's id and
// realm-info; the host discards its attributes, relationships and
// `included[]`. So the write path skips the `loadLinks` closure —
// a linked card is NOT inlined into the create response — while a
// subsequent GET, on the read path, still assembles it.
let target = await request
.post('/')
.send({
data: {
type: 'card',
attributes: { firstName: 'Target' },
meta: {
adoptsFrom: { module: rri('./friend.gts'), name: 'Friend' },
},
},
} as LooseSingleCardDocument)
.set('Accept', 'application/vnd.card+json');
assert.strictEqual(target.status, 201, `HTTP 201: ${target.text}`);
Comment on lines +1824 to +1836

@richardhjtan richardhjtan Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Claude Code 🤖] This 500s on the first POST in CI, so the test never reaches the included assertions it exists to make — the contract it is here to pin is unpinned.

rri('./friend.gts') resolves against the created card's own URL, and a POST to / lands the card at <realm>/Friend/<uuid>.json, so the ref resolves to <realm>/Friend/friend.gts: fileSerialization throws FilterRefersToNonexistentTypeError (Module entry not found for URL) and the handler turns it into a 500. The other POST-to-/ tests in this file spell the ref as a prefixed module (rri('@cardstack/base/card-api')).

While you're in here: both PATCH echoes change too, and neither the write branch nor the no-op short-circuit has a test asserting the closure is absent.

Regression, blocking.

let targetId = (target.body as SingleCardDocument).data.id!;

let response = await request
.post('/')
.send({
data: {
type: 'card',
attributes: { firstName: 'Consumer' },
relationships: {
friend: { links: { self: targetId } },
},
meta: {
adoptsFrom: { module: rri('./friend.gts'), name: 'Friend' },
},
},
} as LooseSingleCardDocument)
.set('Accept', 'application/vnd.card+json');
assert.strictEqual(
response.status,
201,
`HTTP 201: ${response.text}`,
);

let json = response.body as SingleCardDocument;
// Everything the client keeps from a write response is present...
assert.ok(json.data.id, 'the create response carries the new id');
assert.ok(
json.data.meta.realmInfo,
'the create response carries realm-info',
);
assert.ok(
json.data.meta.lastModified,
'the create response carries lastModified',
);
// ...and the transitive closure it discards is absent.
assert.strictEqual(
json.included,
undefined,
'the create response does not inline the linked card into included[]',
);

// The read path is unchanged: a GET of the same card still assembles
// the closure, so the linked card comes off the wire in included[].
let read = await request
.get(`/${json.data.id!.slice(testRealmHref.length)}`)
.set('Accept', 'application/vnd.card+json');
assert.strictEqual(read.status, 200, `HTTP 200: ${read.text}`);
assert.ok(
(read.body.included ?? []).some(
(r: { id?: string }) => r.id === targetId,
),
'the GET response still inlines the linked card into included[]',
);
});

test('an echoed serve-time meta.screenshots never persists into the source file', async function (assert) {
// The shape a card+json GET stamps — a client that GETs a doc and
// POSTs it back to duplicate the card echoes this, and persisting
Expand Down Expand Up @@ -3273,11 +3347,12 @@ module(basename(import.meta.filename), function () {
);
});

test('PATCH response carries an ETag and writes invalidate the previous one', async function (assert) {
// Capture the pre-patch ETag, mutate the card, and verify the PATCH
// response advertises a *different* ETag for the new state — that's
// the contract that lets the caller cache the post-patch body
// without an extra round-trip GET.
test('PATCH response omits the ETag; the write still advances the GET validator', async function (assert) {
// The write echo omits the link closure a GET assembles, so it must
// NOT carry the GET's validator — a client caching the echo body and
// revalidating with its ETag would be 304'd onto a closure-less
// representation. The write still rotates the validator a GET
// reports, so a follow-up conditional GET behaves correctly.
let initialResponse = await request
.get('/person-1')
.set('Accept', 'application/vnd.card+json');
Expand All @@ -3301,16 +3376,9 @@ module(basename(import.meta.filename), function () {
.set('Accept', 'application/vnd.card+json');

assert.strictEqual(patchResponse.status, 200, 'PATCH succeeds');
let patchEtag = patchResponse.get('etag') ?? '';
assert.ok(patchEtag, 'PATCH response carries an ETag');
assert.true(
/^"\d+(?:-[0-9a-f]+)?:card-rri"$/.test(patchEtag),
`PATCH ETag matches "<indexed_at>(-<realmInfoHash>)?:card-rri" pattern (got ${patchEtag})`,
);
assert.notStrictEqual(
patchEtag,
originalEtag,
'PATCH advances the ETag because indexed_at bumps on the rewrite',
assert.notOk(
patchResponse.get('etag'),
'the write echo carries no validator',
);

// Sending the OLD etag against If-None-Match must NOT short-circuit
Expand All @@ -3324,25 +3392,31 @@ module(basename(import.meta.filename), function () {
200,
'old ETag no longer matches → fresh 200',
);
assert.strictEqual(
staleResponse.get('etag'),
patchEtag,
'GET reports the new ETag',
let newEtag = staleResponse.get('etag') ?? '';
assert.ok(newEtag, 'GET reports a validator');
assert.true(
/^"\d+(?:-[0-9a-f]+)?:card-rri"$/.test(newEtag),
`GET ETag matches "<indexed_at>(-<realmInfoHash>)?:card-rri" pattern (got ${newEtag})`,
);
assert.notStrictEqual(
newEtag,
originalEtag,
'the write advanced the GET validator because indexed_at bumps on the rewrite',
);

// And the new etag from the PATCH must short-circuit on next GET.
// And the advanced validator must short-circuit on the next GET.
let cachedResponse = await request
.get('/person-1')
.set('Accept', 'application/vnd.card+json')
.set('If-None-Match', patchEtag);
.set('If-None-Match', newEtag);
assert.strictEqual(
cachedResponse.status,
304,
'new ETag from PATCH lets a follow-up GET short-circuit',
'the advanced validator lets a follow-up GET short-circuit',
);
});

test('no-op PATCH response carries an ETag matching the existing one', async function (assert) {
test('no-op PATCH response omits the ETag; the GET validator is unchanged', async function (assert) {
// Prime once so the stored file is in canonical serialized form;
// the no-op assertions below measure the steady state (see the
// no-op lastModified test).
Expand Down Expand Up @@ -3382,10 +3456,20 @@ module(basename(import.meta.filename), function () {
.set('Accept', 'application/vnd.card+json');

assert.strictEqual(patchResponse.status, 200, 'no-op PATCH succeeds');
assert.strictEqual(
assert.notOk(
patchResponse.get('etag'),
'the no-op write echo carries no validator either',
);

// The no-op didn't rewrite the file, so a GET still reports the same
// validator it did before the PATCH.
let afterResponse = await request
.get('/person-1')
.set('Accept', 'application/vnd.card+json');
assert.strictEqual(
afterResponse.get('etag'),
initialEtag,
'no-op PATCH returns the same ETag (no rewrite, indexed_at unchanged)',
'the GET validator is unchanged (no rewrite, indexed_at unchanged)',
);
});

Expand Down
85 changes: 33 additions & 52 deletions packages/runtime-common/realm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7743,12 +7743,15 @@ export class Realm {
lastModified,
);
} else {
// The write response is read only for the primary card's assigned id
// and realm-info; the client discards its attributes, relationships and
// `included[]` (see `persistAndUpdate` in host store.ts). Skip the
// transitive `loadLinks` closure and query-backed expansion — assembling
// a link graph nothing reads is wasted work (the closure walk is the
// bulk of read-handler time), and side-loaded children round-trip their
// ids through their `lid`s, not this response.
Comment on lines +7746 to +7752

@richardhjtan richardhjtan Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Claude Code 🤖] "the client discards its attributes, relationships and included[]" understates the change: loadLinks also writes relationships.<field>.data onto the primary resource (entry.relationship.data = …), so the echo now carries relationships with links.self and no resource linkage. That is a different document for the written card itself, not only a missing included[] — and to a JSON:API client a relationship without data reads as "linkage unknown" rather than "no target".

That is what both host failures are. card-copy-test.gts deep-equals data.relationships.pet including its data member and then dereferences json.included[0]; the interact submode … new linked card is created in a different realm acceptance test deep-equals relationships['friends.1'] with its data member. The copy one surfaces as unable to save copied card instance rather than a failed assertion because the TypeError on included[0] is thrown inside the onSave subscriber, which runs inside persistAndUpdate's try — store.create returns a CardErrorJSONAPI and copy-card.ts throws on it.

Ask: update both tests to the new shape, and reword this comment, its twin on the PATCH branch, and the description's "relationships … returned exactly as before" to say the relationships lose their linkage too.

Regression, blocking.

let entry = await this.#realmIndexQueryEngine.cardDocument(
new URL(newURL),
{
loadLinks: true,
skipQueryBackedExpansion: false,
},
);
if (!entry || entry?.type === 'error') {
let err = entry
Expand Down Expand Up @@ -7803,8 +7806,7 @@ export class Realm {
let duringPrerender = isDuringPrerenderRequest(request);
// A skip-index-wait caller (see SKIP_INDEX_WAIT_HEADER) takes the same
// write-side path as a prerender write — index deferred, answer from the
// serialized echo — without the prerender-only serialization tweaks
// (skipQueryBackedExpansion) that stay gated on `duringPrerender` below.
// serialized echo rather than a readback.
let answerFromEcho = duringPrerender || isSkipIndexWaitRequest(request);

let { data: patch, included: maybeIncluded } = await request.json();
Expand Down Expand Up @@ -7945,12 +7947,12 @@ export class Realm {
// If the patch makes no semantic changes and doesn't include side-loaded
// resources, short-circuit to avoid touching the file (and changing mtime).
if (included.length === 0 && isEqual(primaryResource, original)) {
// No links closure: the PATCH response is read only for the primary
// card's id and realm-info, and this readback runs inside the
// realm-wide write lock every other writer queues on (see the
// non-short-circuit readback below for the full rationale).
let entry = await this.#realmIndexQueryEngine.cardDocument(
new URL(instanceURL),
{
loadLinks: true,
skipQueryBackedExpansion: duringPrerender,
},
);
if (entry && entry.type !== 'error') {
let existingDoc = merge({}, entry.doc, {
Expand All @@ -7962,11 +7964,10 @@ export class Realm {
let createdAt = await this.getCreatedTime(
this.paths.local(url) + '.json',
);
// The PATCH echo is the same served representation as a GET —
// including the joined `meta.screenshots` (the store replaces an
// instance's meta wholesale from a save response, so an echo
// without it would wipe the key client-side until the next GET)
// and the same validator components.
// The PATCH echo carries the joined `meta.screenshots` a GET would
// (the store replaces an instance's meta wholesale from a save
// response, so an echo without it would wipe the key client-side
// until the next GET).
if (entry.screenshots) {
existingDoc.data.meta = {
...existingDoc.data.meta,
Expand All @@ -7976,27 +7977,18 @@ export class Realm {
}),
};
}
// entry.doc came from cardDocument(), which already called
// attachRealmInfo() and (re)populated the realm-info cache —
// so the cached hash is current as of this response.
await this.getRealmInfo();
let foreignDeps = this.hasForeignRealmDeps(entry.deps);
let etag = foreignDeps
? undefined
: buildCardJsonEtag(
entry.indexedAt,
this.getCachedRealmInfoHash(),
screenshotsEtagFingerprint(entry.screenshots),
);
// No validator on the write echo: the echo omits the link closure a
// GET assembles, so it is a different representation than the GET
// whose ETag it would otherwise share — emitting one would let a
// conditional GET 304 onto this closure-less body. The client
// discards the echo body anyway, so it has no validator to gain.
this.#serveInstanceIdsAsRRI(existingDoc);
return createResponse({
body: JSON.stringify(existingDoc, null, 2),
init: {
headers: {
'content-type': SupportedMimeType.CardJson,
'cache-control': this.cardJsonCacheControl(requestContext),
...(etag ? { etag } : {}),
...etagSuppressedHeader(foreignDeps),
...lastModifiedHeader(existingDoc),
...(createdAt != null
? { 'x-created': formatRFC7231(createdAt * 1000) }
Expand Down Expand Up @@ -8101,12 +8093,15 @@ export class Realm {
requestContext,
});
}
// The write response is read only for the primary card's assigned id
// and realm-info; the client discards its attributes, relationships and
// `included[]` (see `persistAndUpdate` in host store.ts). Skip the
// transitive `loadLinks` closure and query-backed expansion — assembling
// a link graph nothing reads is wasted work, and here it is wasted
// inside the realm-wide write lock every other writer on this realm is
// serialized behind.
let entry = await this.#realmIndexQueryEngine.cardDocument(
new URL(instanceURL),
{
loadLinks: true,
skipQueryBackedExpansion: false,
},
);
if (!entry || entry?.type === 'error') {
if (
Expand Down Expand Up @@ -8155,32 +8150,18 @@ export class Realm {
};
}
}
// Same rationale as the no-op short-circuit branch above:
// cardDocument() above primed the realm-info cache via
// attachRealmInfo(), but only when entry was a non-error doc.
// On the error fallback we may still need to populate it.
await this.getRealmInfo();
let foreignDeps =
entry && entry.type !== 'error'
? this.hasForeignRealmDeps(entry.deps)
: false;
let etag =
entry && entry.type !== 'error' && !foreignDeps
? buildCardJsonEtag(
entry.indexedAt,
this.getCachedRealmInfoHash(),
screenshotsEtagFingerprint(entry.screenshots),
)
: undefined;
// No validator on the write echo (same rationale as the short-circuit
// branch above): the echo omits the link closure a GET assembles, so it
// is a different representation than the GET whose ETag it would share —
// emitting one would let a conditional GET 304 onto this closure-less
// body. The client discards the echo body anyway.
this.#serveInstanceIdsAsRRI(doc);
return createResponse({
body: JSON.stringify(doc, null, 2),
init: {
headers: {
'content-type': SupportedMimeType.CardJson,
'cache-control': this.cardJsonCacheControl(requestContext),
...(etag ? { etag } : {}),
...etagSuppressedHeader(foreignDeps),
...lastModifiedHeader(doc),
...(created ? { 'x-created': formatRFC7231(created * 1000) } : {}),
},
Expand Down
Loading