fix(engine): run the roll-up summary recompute under a system context - #7776
Conversation
…#7673) `recomputeSummaries` issued the parent roll-up write under the CALLER's execution context, so an engine-derived write was authorized as if the caller had asked for it. On the ordinary parent/child shape — a child more widely writable than its parent (tasks, line items, comments, time entries) — a GRANTED child write returned HTTP 500 after the row had already committed: the parent update raised PERMISSION_DENIED, which the call site rethrew as SummaryRecomputeError (ERR_SUMMARY_RECOMPUTE) and REST maps to a 500. A client that retried created a duplicate row. The recompute is now system-elevated, covering all three call sites (insert, update, delete) through the single seam they share. The elevation is a sudo()-shaped derivative of the caller's context, so an open transaction handle, tenantId and timezone still ride along — the same posture the roll-up's two other writers (the insert-time seed and the summary-nulls backfill) already held. Two quieter defects go with it, both visible only where the caller COULD write the parent: the aggregate was computed over the caller's row-level-visible subset (storing one reader's view of the collection on the parent), and an author-declared `readonly: true` roll-up column was dropped by the write-path read-only strip, which runs on `!context.isSystem`. The elevation does not widen what a caller may read or write: the parent's row stays governed by the caller's grants, the summary column stays subject to the parent's FLS on read, and ERR_SUMMARY_RECOMPUTE still surfaces genuinely failed recomputes, which is what the seed loader and import runner branch on. Tests: a shared consistency (engine-summary-recompute-context.test.ts) runs every assertion over all three call sites via it.each — the write resolves, the roll-ups land the recomputed values, the parent write carries isSystem, the child write stays under the caller context, and the aggregate reads the whole child collection — plus refusal controls asserting a direct caller update of the parent is still refused with code PERMISSION_DENIED and status 403. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxnTmZY88mwebsRW9DXiV3
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 15 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
Fixes #7673
What was wrong
recomputeSummariesissued the parent roll-up write under the caller's execution context, so an engine-derived write was authorized as if the caller had asked for it. On the ordinary parent/child shape — a child more widely writable than its parent (tasks, line items, comments, time entries) — a granted child write returned HTTP 500 after the row had already committed: the parent update raisedPERMISSION_DENIED, the engine recorded it as a recompute failure, and the call site rethrew it asSummaryRecomputeError/ERR_SUMMARY_RECOMPUTE, which REST maps to a 500. A client that retried created a duplicate row (#7673, and #7719 which reported thePATCHhalf).The access matrix and
/security/explainboth saidcreate: true, so a declared-and-granted operation failed on a permission check about a record the caller never asked to touch.The fix
The recompute now runs system-elevated, at the single seam its three call sites share (insert / update / delete), so no call site can drift from the others. A roll-up is engine-derived state, not a caller write: the permission decision that matters — may this caller write the child — has already been made by the time the recompute runs.
The elevation is a
sudo()-shaped derivative of the caller's context ({ ...execCtx, isSystem: true }), never a bare{ isSystem: true }, so an open transaction handle,tenantIdandtimezonestill ride along. That makes this the same posture the roll-up's two other writers already held — the insert-time seed (initializeSummaryFields) and thesummary-nullsbackfill (which elevates explicitly, #6063) — so all three writers of a summary column now agree about who owns it.Two quieter defects fixed with it
Both were invisible on the reported repro because the recompute never got that far; they showed only where the caller could write the parent and the recompute therefore "succeeded":
readonly: trueroll-up column was dropped by the write-path read-only strip, which runs on!context.isSystem— so that summary never landed at all.What this deliberately does NOT change
updateof the parent is refused exactly as before — and the summary column stays subject to the parent's field-level security on read. The only value this path can move is the one the author declared as a function of the child collection.ERR_SUMMARY_RECOMPUTEis unchanged. It still surfaces a genuinely failed recompute (a driver or network failure that outlives its retries), which is whatmetadata-protocol's seed loader andrest's import runner branch on. Only the permission-denial cause is removed, by making it unreachable.withTransientRetryis untouched — see the measurement note below.Measurement notes
Two of the mechanism hypotheses on the card did not survive contact with
origin/main, and the implementation follows what was measured:withTransientRetrydoes not retry a permission denial.defaultIsTransientErrormatches neither the localizedpermission_deniedsentence norcode: 'PERMISSION_DENIED', so the denial threw on the first attempt.engine-summary-retry.test.tsalready pinned this from the other side ("does not retry a non-transient parent-update failure"). The×2warn in the issue's log is two roll-up fields on one parent (task_count,total_estimate), not two retries. No change was needed here, and none was made.ERR_SUMMARY_RECOMPUTEcases are genuine failures whose surfacing is a deliberate framework#3147 decision that two consumer packages branch on. Changing it would be a separate contract change, out of this card's scope.Tests
packages/objectql/src/engine-summary-recompute-context.test.ts— a shared consistency, not a single-path regression test.recomputeSummarieshas three call sites implementing one contract, and the symptom was reported on two of them, so every assertion runsit.each(WRITE_PATHS)over insert, update and delete:ERR_SUMMARY_RECOMPUTE);isSystem— theplugin-securitybypass the fix relies on — and still carries the caller's identity, proving the elevation is a derivative rather than a bare system context;Plus refusal controls in the ADR-0112 envelope (
codeandstatusCode, never a baretoThrow): a direct caller update of the parent is still refusedPERMISSION_DENIED/ 403 and leaves the row untouched, including immediately after a child write moved the derived column. A repointing update is covered separately — both old and new parent recompute, both elevated.The fixture mirrors the reported shape (
showcase_project.task_count+total_estimateovershowcase_task), with sibling rows owned by another user so the expected values are only reachable if the aggregate saw the whole collection.Reverse verification (predicted before running: the three write paths go red with
ERR_SUMMARY_RECOMPUTE; the pure refusal controls stay green, since they do not depend on the fix). With the two-line elevation reverted, 17 of 19 red, and the serialized failure reproduces the issue verbatim:The two that stayed green are exactly the refusal controls — correct, because a scoping control must be insensitive to the fix in both directions.
Pin sweep
git grepoverSummaryRecomputeError/ERR_SUMMARY_RECOMPUTEacross every consumer layer: no pin of the old outcome exists to flip. The live pins all cover the driver-failure path, which is unchanged and still green —packages/objectql/src/engine-summary-retry.test.ts(transient / non-transient parent-update failures),packages/metadata-protocol/src/seed-loader-retry.test.tsandseed-loader-summary-stale.test.ts,packages/rest/src/import-runner-idempotency.test.ts(all three synthesize the error rather than provoke it). The two docs hits are generated error-code ledgers listing a code that still exists.Verification
pnpm --filter '@objectstack/objectql^...' buildpnpm --filter @objectstack/objectql testpnpm --filter @objectstack/objectql typecheckpnpm --filter @objectstack/plugin-security --filter @objectstack/rest --filter @objectstack/metadata-protocol testpnpm check:adr-anchors·check:durability-log-level·check:engine-double-contract·check:stack-collection-maps·check:nul-bytes·node scripts/check-engine-split-ratio.mjsConsumer sweep direction: the three consumer packages above were selected by implication (the gate being bypassed, the layer that maps the 500, the layer that recovers the code) rather than by a full prefix sweep of all 40 downstream packages — this is a behavioural change behind one context flag with no exported-type change, so the type-narrowing rationale for a full downstream sweep does not apply here.
Docs:
content/docs/data-modeling/fields.mdxnow states the authorization posture for roll-ups explicitly, next to the existing "summary fields are server-owned" sentence.Generated by Claude Code