CS-11010: ETag/304 for application/vnd.card+json#4683
Conversation
GET /<card> with Accept: application/vnd.card+json now emits an ETag of the form "<indexed_at>:card", matching the existing "<base>:<variant>" pattern that modules and source files use. On a matching If-None-Match the handler short-circuits to 304 before the loadLinks fan-out, which is the bulk of the per-request server cost on a card with many included resources. indexed_at is the right base because it bumps on every reindex — direct file writes AND dependency-triggered re-writes via the deps graph — so a change to any included card invalidates the parent's cache. last_modified alone wouldn't (it's the file mtime, untouched by dep-only reindexes), and an md5 of the assembled body would be correct but couldn't be computed without paying the assembly cost we're trying to skip. Cache-Control follows the auth model: world-readable realms get `public, max-age=0, must-revalidate` so a CDN can revalidate; auth- gated realms get `private` so a shared cache can't serve one user the response of another. PATCH responses (both the no-op short-circuit and the post-write path) now also carry the ETag, so a caller that uses the PATCH response body can immediately revalidate against it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
💡 Codex Reviewboxel/packages/runtime-common/realm.ts Lines 3391 to 3392 in f4123b2  Include all assembled response inputs in the ETag
When a card is revalidated after data that is assembled outside the primary instance row changes, this can return a stale 304. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
habdelra
changed the title
habdelra
changed the title
Host Test Results 1 files + 1 1 suites +1 1h 59m 37s ⏱️ + 1h 59m 37s Results for commit a0a659a. ± Comparison against earlier commit dacb2c8. Realm Server Test Results 1 files ± 0 1 suites +1 17m 28s ⏱️ + 17m 28s Results for commit a0a659a. ± Comparison against earlier commit dacb2c8. |
Codex flagged that `cardDocument()` adds `meta.realmInfo` at request time via `attachRealmInfo()`, drawing from a cached `getRealmInfo()` that nulls on `/_config` PATCH and on publish without bumping any card's `indexed_at`. With the previous `<indexed_at>:card` ETag, a client revalidating with the pre-PATCH ETag would 304 onto a body whose `realmInfo.name` (or `iconURL`, `lastPublishedAt`, etc.) had changed. The fix folds an md5 hash of the cached realmInfo into the ETag base: `<indexed_at>-<realmInfoHash>:card`. The hash is computed lazily inside `getRealmInfo()` and tracked in a sibling `#cachedRealmInfoHash` field that's nulled wherever `#cachedRealmInfo` was — wrapped in a tiny `invalidateCachedRealmInfo()` helper so the two always move together. The card+json handlers (GET, PATCH) prime the cache via `getRealmInfo()` before computing the ETag, then read the hash through `getCachedRealmInfoHash()`. A new `buildCardJsonEtag()` helper centralizes the format. Existing realm-endpoints-test "Cache-Control header for card json" expectation updated to the new public/max-age=0/must-revalidate contract; new regression test exercises the scenario Codex described — old ETag must not 304 after a /_config name change. Cross-realm linked resources are unaffected: `attachRealmInfo()` only stamps THIS realm's info on resources whose `meta.realmURL` matches this realm. Foreign resources' realmInfo isn't injected here, and instance-level changes on those resources still propagate via the dependency graph (which bumps this card's `indexed_at`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex you're right — pushed Mechanics: Added a regression test exactly for the case you called out: capture pre-PATCH ETag → On the other half of your concern — cross-realm linked resources loaded at request time — I think that case is already covered, but checking my reasoning: |
|
Summary
Testing
|
Codex follow-up to the previous commit. Cross-realm dep invalidation is not currently wired up in `index-writer.calculateInvalidations` — the SQL filters dependents by `realm_url = $thisRealm` (its own comment flags this: "probably need to reevaluate this condition when we get to cross realm invalidation"). Practical consequence: when card X in realm A `linksTo` card Y in realm B, a write to Y bumps B's `indexed_at` for Y but never propagates to A's `indexed_at` for X. Meanwhile `loadLinks` for X re-fetches Y over HTTP at every request and surfaces Y's new content. Without a guard, A's ETag-based 304 path would serve a body whose `included[]` is stale. Fix: detect foreign-realm deps on the primary instance row and suppress ETag emission for that card entirely — both the 304 short-circuit and the response header in the GET 200, no-op PATCH, and post-write PATCH paths. `SearchResultDoc` exposes the deps array so the PATCH paths can run the same check without an extra `instance()` query. The check ignores module-graph deps (non-http URLs, registered prefixes, and `cardstack.com/base/` paths) so platform code references — which every card has — don't blanket-suppress the ETag. Only foreign card *instance* URLs trigger the guard. When cross-realm invalidation lands, this guard can be removed and ETags will be correct for cards with foreign instance deps too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Good catch — pushed WHERE deps @> [...]
AND i.realm_url = $thisRealm -- "probably need to reevaluate this condition when we get to cross realm invalidation"So a write to a foreign-realm card never bumps the consuming realm's Implementation:
If left as-is those would mean every card has a "foreign" dep (every card adopts from
I didn't add the integration test you described in this commit — a multi-realm fixture is heavy enough that I'd rather get the guard reviewed first and add the test separately if reviewers want it. Happy to wire one through Followup work flagged: when cross-realm dep cascading actually lands (the comment in |
Multiple correctness and quality fixes across the ETag work:
- Resolve registered prefixes when classifying foreign deps. Production
realms register every realm via addRealmMapping, so deps are stored
in prefix form (@cardstack/foreign-realm/foo.json). The previous
dep.startsWith('http') early-return classified those as not-foreign
and the guard silently failed. New isForeignRealmDep resolves the
dep first, then trips on .json instance deps that resolve outside
this realm — including base-realm card instances. Module/scoped-CSS
deps still pass.
- Invalidate the source realm's #cachedRealmInfo on publish/unpublish.
The lastPublishedAt map in RealmInfo is built from realm_registry
rows joined on source_url, so (un)publishing a derivative changes
it without otherwise touching the source — without the hook, the
source's card+json ETag would 304 against pre-publish validators
forever.
- Pin Postgres row order in querySourceRealmPublications with
ORDER BY url. Without it, JSON.stringify of Object.fromEntries(rows)
is non-deterministic across instances/restarts and the realm-info
hash flips for the same logical state — every browser cache
invalidates on cache-prime races.
- Quote the ETag value ("<base>:card") per RFC 9110 §8.8.3 — CDNs
and browsers don't re-quote inbound validators.
- Parse If-None-Match as a list with weak-prefix tolerance and *
per RFC 9110 §13.1.2. Strict-equality matching missed common
validators (CDN-injected W/, comma lists).
- Move the .json redirect to the top of getCard so the canonical URL
is the only one that ever serves an ETag — keeps client/server
cache keys aligned and prevents intermediaries from caching a 304
bound to the .json form.
- Skip the instance() peek when the request has no If-None-Match.
cardDocument() does its own instance lookup, so the peek was a
redundant DB round-trip on the cache-miss path.
- Re-derive hasForeignDeps from the cardDocument() result for the
response header path. The early peek's snapshot may differ from
the assembly's snapshot — using the assembly's deps closes a
write-race hole.
- Update the CARD_JSON_ETAG_VARIANT comment to describe both inputs
(was outdated after the earlier realmInfo fix). Update the
cardJsonCacheControl comment to acknowledge must-revalidate is
added on top of the source/module pattern, not the same.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed Correctness
RFC compliance
Race fix
Layout
Comments
Tests updated to match the now-quoted format. The earlier CI red on Still open: integration test for cross-realm dep ETag-suppression. Happy to add via |
Summary
Checks
|
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
I'm not crazy about quietly skipping etags when there is a cross realm dependency. It seems like it could lead to unexpectedly uneven performance experiences. I guess we can fix it when we add cross-realm invalidation?
|
Thanks for the review @lukemelia. Good call on the unevenness concern — I agree it's a real foot-gun: two cards in the same realm could have very different perf profiles (one 304s, the other re-assembles every time) and that surface area only widens as more realms link cross-realm. The suppression is meant as a correctness floor while we still have the realm-bounded Two ways I'd consider making it less quiet in the meantime, if it's worth a small follow-up:
Happy to do (1) as a tiny follow-up to this PR if you'd like — it's ~5 lines and gives us a measurable signal that drives the case for cross-realm invalidation. Or fold it into the cross-realm-invalidation work and remove both at the same time. |
Card+json responses whose ETag is suppressed by the foreign-realm- deps guard now carry `X-Boxel-Etag-Suppressed: foreign-deps`. The header is informational — it doesn't change client behavior — but it makes the suppression visible to ops dashboards and log aggregation. Per @lukemelia's review concern about uneven perf profiles being silently invisible: this gives us a measurable signal we can plot in Grafana to drive prioritization of the cross-realm dep invalidation work in `index-writer.calculateInvalidations`. Once that lands, both the suppression AND the header come out together. Applied to all three response sites that participate in the guard: PATCH no-op short-circuit, PATCH post-write, and GET 200. The 304 fast-path doesn't need it — when foreign deps fire there we never emit an ETag in the first place, so we fall through to the 200 path which carries the signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Done — pushed Applied to all three response sites that participate in the guard (PATCH no-op, PATCH post-write, GET 200) plus a regression assertion that the header is absent on a card with only local deps so dashboards can tell normal from suppressed traffic. |
Closes CS-11010. Companion to #4667 (CS-11037).
How this fits with #4667
#4667 lets a
_federated-searchcaller sendinclude: []to skip the server-side side-loading of relationships entirely. The big win there is the cards-grid prerender path: 87% of staging render-timeouts trace to that one query shape, each timeout burning the full 90 s budget, because the server fan-outs N×M sequential link queries even when the consumer reads onlyinstance.id/instance.constructor.But once a caller opts out of side-loading, any other relationship it later wants has to be fetched individually as
GET /<card>withAccept: application/vnd.card+json. Today every one of those individual GETs is served withcache-control: no-store, no-cache, must-revalidateand a fresh full-body response. A page that does N follow-up link fetches pays the realm's full ~100 ms JSON-assembly cost on every page view, every repeat visit, even when nothing changed.This PR closes that loop:
_federated-searchreturnsdata[]only, noincluded[], noloadLinkswork.GET /<card>for a linked card gets an ETag and short-circuits to 304 on a warm cache.So the cost shifted off
_federated-searchin #4667 doesn't reappear as N unconditional GETs on the very next render — it lands in the browser cache and stays there until the underlying card actually changes.Scope difference vs. how #4667's description framed CS-11010
#4667's "Companion work" section described CS-11010 as making
vnd.card+jsonETag-aware on published realms. After looking at it, I went broader: this PR applies to all realms. Reasoning is in the ticket comment — modules and source files are already ETagged in every context (interactive, auth-gated, mutating realms included), so cards should follow the same convention. There's no realm class where instance data has a fundamentally different cacheability story than its module/source siblings.Behavior
indexed_at, and short-circuits to 304 before the expensiveloadLinksfan-out. Cache-hit cost drops from ~100 ms+ to ~5 ms."<indexed_at>:card". Matches the existing<base>:<variant>convention fromMODULE_ETAG_VARIANT(<lastModified>:module) andSOURCE_ETAG_VARIANT(<contentHash>:source) inruntime-common/realm.ts.public, max-age=0, must-revalidatefor world-readable realms,private, max-age=0, must-revalidatefor auth-gated ones, mirroring the source/module file path.privatekeeps a shared cache (CDN, intermediate proxy) from serving one user's response to another.Why
indexed_atis the right ETag baseThe
indexed_atcolumn on a card'sboxel_indexrow is set on every reindex. The realm's dependency-invalidation chain (thedepsarray on each row) means any change that affects the assembled card+json document — including a write to a transitively-linked card — triggers a reindex and bumpsindexed_aton the dependent rows. Soindexed_atis a complete fingerprint for the assembleddata + included[]document.Two alternatives I considered and rejected:
last_modifiedalone (the file's mtime). Untouched by dep-only reindexes, so a 304 here would serve staleincluded[]data after a linked card was updated. Wrong.indexed_atis the cheap, correct middle: one column read on the primary row, computed beforeloadLinksruns.Wire path
SearchResultDoc(inrealm-index-query-engine.ts) gains anindexedAt: number | nullfield so the PATCH path can emit the ETag from a singlecardDocumentcall without a redundantinstancelookup.Test plan
pnpm lintclean (runtime-common, includeslint:js+lint:types).packages/realm-server/tests/card-endpoints-test.ts. Local runs of the full card-endpoints suite were blocked by an unrelated environmental flake on this machine ("No standby page available for prerender" — Puppeteer/Chrome can't open new tabs, almost certainly an Ubuntu 24.04 + AppArmor-restricted-userns interaction). The new test cases themselves are scoped to request/response contracts and don't depend on prerender behavior; CI should exercise them cleanly.New test cases
GET, public-readable realm:
returns an ETag and public cache-control on a 200 responsereturns 304 when If-None-Match matches the current ETag— verifies status, ETag echo, cache-control passthrough, and empty bodyreturns 200 when If-None-Match does not matchGET, auth-gated realm:
200 with permission(extended) — assertsprivatecache-control and ETag presencePATCH, public-writable realm:
PATCH response carries an ETag and writes invalidate the previous one— verifies (a) post-write ETag differs from pre-write, (b) the old ETag no longer 304s after the write, (c) the new ETag does 304 a follow-up GETno-op PATCH response carries an ETag matching the existing one— confirms the no-op short-circuit doesn't invent a new ETag🤖 Generated with Claude Code