Measure card write size per-file, not per whole request body - #6160
Conversation
A card write POSTs a JSON:API document whose `included[]` inlines every linked card the tab has resident. The realm discards every included member without a `lid` on write (it keeps only the primary card plus any brand-new, unsaved links being created in the same request), then holds each resulting file to the card size limit individually. The client-side size check measured the entire concatenated request body instead, so a new card whose own document was a few KB could fail "Card size exceeds maximum allowed size" purely because of how much of the realm the tab happened to have loaded into `included[]` — a silent, session-dependent failure whose message named the wrong cause. Validate each resource that will actually become a file on its own — the primary card plus only the `lid`-bearing included members — mirroring the realm's own per-file write-size check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GsYGsuCqHJz9GAti4jheG
Preview deploymentsHost Test Results 1 files 1 suites 2h 7m 35s ⏱️ Results for commit a59c285. Realm Server Test Results 1 files ± 0 235 suites ±0 1h 22m 57s ⏱️ + 1m 12s Results for commit a59c285. ± Comparison against earlier commit 6bf4a83. |
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Review focused on the mirrored-contract claims — the client check against the realm's write-path retention and per-file enforcement — and on empirically pinning the new test to the fix; the surrounding save pipeline was not re-reviewed.
Bottom line: no blocking issues.
Answering the description's open item (browser test run pending): ran the new test against a built dist — passes 3/3 with the fix in place, and fails (the save is rejected) with the validateCardWriteSize call reverted to the old whole-body check. The test pins the change.
One non-blocking recommendation, which lands on the stacked follow-up rather than here: the lid-retention predicate is growing multiple spellings across client and realm — see the inline comment on the follow-up PR for the shared-predicate suggestion.
Adjacent, out of scope: the realm's write handlers silently skip any included member — lid-bearing or not — whose meta.realmURL names a different realm, so a cross-realm co-create is dropped with no error and no file. This PR's over-count for that shape is harmless (the save fails client-side instead of half-succeeding server-side), but whoever next touches the write contract may want a 400 there.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed for whether the client's new per-resource rule actually matches what the realm writes, and for whether the test pins the rule. Didn't run the browser suite.
The split is right and no blocking issue in the production change — the gap is in the test, which passes whether or not the lid filter does anything. Both realm write paths keep exactly the primary plus lid-bearing side-loads (realm.ts card POST handler; card-operations/executors.ts create and update), and each resulting file is held to the ceiling individually, so measuring { data: resource } per resource is the right unit. It measures slightly high against the stored bytes rather than low — file-serializer strips id/lid/included and collapses relationship data into a links.self — so the client can't pass something the realm then 413s.
Recommendations:
- Add the negative case: an oversized unsaved link must still be rejected. See the comment on the new test in
store-test.gts— the current assertions hold if theincludedterm is removed altogether. - Decide whether the foreign-realm skip belongs in the filter, or say in the comment that the client is intentionally stricter. See the comment on
validateCardWriteSize.
CI on this head is still queued, and the description flags the browser run as unconfirmed — recommendation 1 is worth landing before that run, since it's what gives the run something to say.
Adjacent, out of scope: executeAtomicOperations measures a card operation as JSON.stringify(operation.data, …) — the bare resource — while the realm stores the wrapping { data: … } document. It under-measures by the wrapper and two spaces of indent per line. Pre-existing, and the atomic op carries no included, so it doesn't have the bug this PR fixes.
Generated by Claude Code
| // The primary card is always written; an included member is written only | ||
| // when it carries a `lid` (an unsaved link created alongside this card). | ||
| let resources = [ | ||
| doc.data, | ||
| ...(doc.included ?? []).filter( | ||
| (resource) => typeof (resource as { lid?: unknown }).lid === 'string', | ||
| ), |
There was a problem hiding this comment.
[Claude Code 🤖] lid is only half of what the realm skips — it also drops any resource whose meta.realmURL isn't this realm, on both write paths (realm.ts, the card POST handler's resource.meta.realmURL && ensureTrailingSlash(...) !== this.url test in the same loop as the lid test; and namesForeignRealm in card-operations/executors.ts, in both the create and the update side-load loops). So a lid-bearing side-load naming another realm is measured here and discarded there — the client can refuse a write the realm would have accepted.
Narrow in practice, so non-blocking, but the comment above states this mirrors the realm's per-file assertWriteSize, and the next person will build on that. Either add the realm check to the filter, or say in the comment that the client is deliberately the stricter of the two.
Regression class: new logic in this PR, not a pre-existing bug.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Went with the documented-stricter option, in a59c285: the comment now states the foreign-realm lid edge explicitly — measured here, silently skipped (never written) by the realm — and why the client does not reproduce that skip: it is a request defect the realm swallows, not a contract. Matching it would mean faithfully reproducing a silent drop.
| let result = await (storeService as any).persistAndUpdate(instance); | ||
| assert.true( | ||
| isCardInstance(result), | ||
| "a new card whose own document is under the limit saves regardless of how large a graph it links to — the check measures what the realm stores, not the tab's loaded `included`", | ||
| ); | ||
| let cardPath = `${(instance as any).id.substring( | ||
| testRealmURL.length, | ||
| )}.json`; | ||
| assert.ok( | ||
| await testRealmAdapter.openFile(cardPath), | ||
| 'the realm holds the created card', | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] This pins only the permissive half of the new filter. Replace the filter body with () => false — or drop the doc.included term from resources entirely — and this test still passes, because a saved link has no lid either way. Nothing in the suite fails if the lid-bearing side-load stops being measured, which is the case where the client check has to keep holding: an oversized brand-new link created alongside the primary becomes its own file on the realm and gets a 413 there.
Ask: add the counterpart — a small new card whose unsaved link is over the (lowered) ceiling, asserting persistAndUpdate returns a card error naming the size limit. That's what makes the filter's predicate load-bearing in CI rather than just in the comment.
Non-blocking on the fix itself; blocking on the PR's test claim, since as written the new test can't distinguish this fix from having deleted the included measurement outright.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Counterpart test added in a59c285: a card saved alongside an oversized unsaved link is refused, asserting the error names the size limit — the case where dropping the lid measurement would sail through client-side. Ran the mutation you described mentally rather than locally (the author opted to let CI arbitrate this round); if the new test comes back green on CI alongside the existing one, both halves of the filter are pinned.
The regression test pinned only the permissive half of the per-resource filter — nothing failed if lid-bearing side-loads stopped being measured. The counterpart test saves a card alongside an oversized unsaved link and asserts the refusal names the size limit. The filter comment now also states the one edge where the client is deliberately stricter than the realm: a foreign-realm lid side-load is measured here but silently skipped (never written) there — a request defect the realm swallows, not a contract to reproduce. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Problem
A card write POSTs a JSON:API document whose
included[]inlines every linked card the tab has resident. On write the realm discards every included member that has nolid— it keeps only the primary card plus any brand-new, unsaved links being created in the same request — and then holds each resulting file to the card size limit individually (Realm#assertWriteSize, per file).The client-side size check measured the entire concatenated request body instead. So a brand-new card whose own document was only a few KB could fail:
purely because of how much of the realm the tab happened to have side-loaded into
included[]. Because the payload depends on what the tab has loaded, the same save works from one session and fails from another for the same card — a silent, session-dependent failure whose message named the wrong cause.Fix
CardServicenow validates each resource that will actually become a file on its own — the primary card plus only thelid-bearing included members — mirroring the realm's own per-file write-size check. Saved (id-bearing) links, which the realm throws away, no longer count against the limit.This is the correctness half of the work. A follow-up (stacked on this PR) stops serialising and transmitting those discarded links in the first place, cutting wire bytes and serialisation time on every new-card save.
Test
Adds an integration test in
store-test.gts: a small new card that links to a large, resident, already-saved card saves successfully once the linked graph would otherwise overflow the (lowered) limit if inlined.Verification status
ember-tsc: clean (only pre-existing, unrelatedpdfjs-distmodule-resolution errors)🤖 Generated with Claude Code