Skip to content

perf(security,protocol): stop asking the same question twice within one request - #10824

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-10757-dedupe-per-request-queries
Aug 21, 2026
Merged

perf(security,protocol): stop asking the same question twice within one request#10824
os-zhuang merged 2 commits into
mainfrom
claude/issue-10757-dedupe-per-request-queries

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Refs #10757 — tranche 1 of the per-request query program (deterministic de-duplication only; no caching, no staleness, no security surface).

Measured, not projected

One authenticated GET /data/crm_account?$top=1, counted by the platform's own instrument (X-OS-Debug-Timing: jsonServer-Timing: db;dur=…;desc="N queries") on pnpm dev:crm --fresh, admin principal, warm request:

DB queries
origin/main (f4e5d91)24
this branch23
this branch, ?$count=false22

Do not read this as the performance problem being solved. It removes roughly a twelfth of a request's queries, and the ~22 that remain — session resolution, grant resolution, localization, metadata — are what the card's tranche 2 (cross-request caching) and the placement work on objectstack-ai/cloud#1518 are for. Neither of those is addressed here.

What changed

$count=false now skips the COUNT query@objectstack/metadata-protocol.

The parameter was already fully plumbed: declared in the spec (ODataQuerySchema.$count), aliased on the wire ($countcount), reserved out of the implicit-field-filter bucket, arity-checked, boolean-coerced — and then deleted unread by the protocol-key strip in findData, so every paginated list ran engine.count() whether or not the caller wanted a total. No contract change was needed: FindDataResponse.total is already optional() and already documented "if requested".

  • Only an explicit false opts out. An absent $count still counts and still reports total. OData reads absent as "omit", and taking that reading would silently strip total from every existing caller — none send the parameter, all read the number.
  • total is omitted, never estimated. A caller who declined the real number should not be handed a plausible-looking guess; hasMore is still answered from the page (a full page means there may be more), the same page-local rule $search already uses.

A find and its COUNT resolve permission sets once, not twice@objectstack/plugin-security.

findData answers a paginated list with two engine operations and the security middleware runs on both, so select * from sys_permission_set where name in (…) went out twice per list request with identical bindings (queries 21 and 23 of the 24 above). The resolution is now memoized per execution context and retired by any write:

  • keyed on the execution-context object in a WeakMap — that object is built once per request (RestServer.execCtxMemo keys it by req; the runtime/MCP dispatchers assemble one per dispatch) and is collected with it. No TTL, no table keyed by user id, nothing that outlives the caller it was resolved for;
  • a process-wide epoch is bumped on every insert/update/delete the middleware sees, placed ahead of the isSystem bypass so a seeder, a package publish or the auto-org-admin grant invalidates too. Any write by any context retires every entry;
  • a context whose grants are rewritten in place re-resolves (the memo key covers positions, permissions, principalKind and the presence of userId);
  • every caller still gets its own array, so the memo is not a new aliasing channel.

What is left is exactly two reads by one context with no write in between — the find/count pair, and the definition of a duplicate question.

The card's third item is NOT implemented, and the reason is a measurement

The card's direction 4 (sys_user read twice, sys_user_permission_set read twice) is confirmed as a duplicate at the SQL level and rejected as a tranche-1 fix. Both pairs are one better-auth read and one framework read, and in each pair the removable half belongs to tranche 2:

pairfirst readsecond readwhy it cannot collapse here
sys_userq3, better-auth core getSessionq15, resolveUserAuthzGrants step 7, for sys_user.ai_accessThe session payload does not carry ai_access — verified against a live /auth/get-session response. auth-manager.ts:1046 refuses to declare it as a better-auth additionalField on purpose ("better-auth SELECTs explicit columns … a column that may not exist on every env yet → broken auth"). Seeding the framework read from the session is therefore impossible without reversing that decision; removing better-auth's own read is the card's direction 1 (session caching).
sys_user_permission_setq4, customSession.isPlatformAdmin(), limit 50q11, resolveUserAuthzGrants §6, limit 100q11 is the authoritative resolution and cannot go. q4 is the customSession enrichment, which the data path never reads back — removing it is the card's direction 2 ("customSession is redundant work on the data path"), in auth-manager.ts, the file tranche 2 was split out to serialise on.

Attempting either inside this PR would have meant widening better-auth's declared user schema or changing when customSession enriches — both authorization-shaped changes wearing a de-duplication costume, which is precisely what the tranche split exists to keep apart.

Verification

Full log of commands and exit codes is in the report comment on the card. Headlines, at fe87edaac:

  • pnpm --filter @objectstack/plugin-security --filter @objectstack/metadata-protocol testexit 0; 69 + 128 files, 1348 + 1747 tests.
  • Downstream consumers objectql / rest / runtime / clientexit 0; 561 test files.
  • pnpm lint (repo-wide eslint . --no-inline-config) → exit 0.
  • Every gate node scripts/pm/dispatch-gates.mjs derived for this diff → exit 0, including check:type-check-debt --re-measure ("33 ledger entr(ies) re-measured … none above its recorded number") and check:cross-package-test-inputs.

New coverage, because the honest answer to "what already covers this?" was the dedupe: nothing; the invalidation: nothing:

  • packages/plugins/plugin-security/src/permission-set-resolution-memo.test.ts — the dedupe, plus four invalidation guards (system write, write by another context, different context object, grants rewritten in place).
  • packages/metadata-protocol/src/protocol.count-opt-out.test.ts — both wire spellings and the boolean form opt out; absent and true are unchanged.

Both suites were ablated to prove they can fail (mutation confirmed on disk by marker counts; both packages' tests import their subject from source, so no dist rebuild is involved):

ablationresult
memo disabled2 red / 4 green — the two dedupe assertions fall, the invalidation guards hold (they are guards, not evidence)
write-epoch bump removed2 red / 4 green — exactly the two write-invalidation tests
countOptOut forced to false4 red / 5 green — the opt-out block falls, the "unchanged" block holds

Found while measuring, not fixed here

resolveLocalizationContext issues three identicalselect * from sys_setting where namespace = ? per request (queries 16–18). The function batches its own fallback read into one $in, but the settings-service path it prefers calls SettingsService.get() three times and each call runs loadRows(namespace, …) over the whole namespace. That is three reads of one question inside one request — tranche-1 shaped, needing no cache — but the card files localization under its direction 5, so it is left for the split's owner to place rather than smuggled in here.

Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-protocol, @objectstack/plugin-security, touching 14 documentable anchor(s).

21 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json a7ea3289eb605c6681ca6bc904e4ab5f799d6504.

5 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: /data/:object (route, 64 pages)
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 19 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json a7ea3289eb605c6681ca6bc904e4ab5f799d6504packageMentionDocs.

Which tree this was computed on

This run read content/docs from f3d444353442b4a943fbd1ea3f1c26e9594e9778 — the merge of head fe87edaac0ebcecf7ae3851261022f24d2cf5ed0 into base a7ea3289eb605c6681ca6bc904e4ab5f799d6504, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin f3d444353442b4a943fbd1ea3f1c26e9594e9778 && git checkout f3d444353442b4a943fbd1ea3f1c26e9594e9778
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin a7ea3289eb605c6681ca6bc904e4ab5f799d6504 fe87edaac0ebcecf7ae3851261022f24d2cf5ed0 && git checkout -B drift-repro a7ea3289eb605c6681ca6bc904e4ab5f799d6504 && git merge --no-ff fe87edaac0ebcecf7ae3851261022f24d2cf5ed0
node scripts/docs-audit/affected-docs.mjs --json a7ea3289eb605c6681ca6bc904e4ab5f799d6504

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs a7ea3289eb605c6681ca6bc904e4ab5f799d6504 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 21, 2026
@os-zhuang

Copy link
Copy Markdown
ContributorAuthor

PM rulings: Q1 → A and B (B is filed as its own card). Q2 → A, filed. And direction 4's premise was mine, and it was wrong.

The correction I owe this PR

I dispatched tranche 1 with three items and called #4 "the duplicate reads … same request, same context, same answer. Remove the second read of each." That framing was wrong, and you disproved it with a live statement capture rather than accepting it:

  • sys_user: better-auth's session payload does not carry ai_access — verified against a live /api/v1/auth/get-session — because auth-manager.ts:1046deliberately refuses to declare it as an additionalField ("better-auth SELECTs explicit columns, so declaring it here would make getSession query a column that may not exist on every env yet → broken auth"). The framework's read cannot be seeded from the session without reversing a documented decision.
  • sys_user_permission_set: the second read is customSession enrichment the data path never reads back. Removing it is direction 2 — in auth-manager.ts, the exact file tranche 2 was split out to serialise on.

So both "free wins" were one better-auth read plus one framework read, and the removable half belongs to tranche 2. Declining to implement them — "authorization-shaped changes wearing a de-duplication costume" — is precisely what 裁决 #3 asked for, and the one path by which tranche 1 could have shipped a silent security bug behind a green suite.

Also noted: seedEmail and the getUserRow memo already exist on main. The card was traced at cloud's pin and main has moved in its favour. Worth remembering the next time a card's trace is treated as current.

Q1 — direction 4: A, and B is filed as cloud-side card #10825

A is right: nothing remains of direction 4 that directions 1 and 2 do not already carry. Fold it in and stop tracking it separately.

B is more than "also worth doing" — it may be the highest-leverage no-risk work left, and the reason comes from a measurement taken after this card was written. cloud#1539 established causally (latency injection, R²=0.9994) that a request is 23.4 sequential legs, and that:

L, not N, is the multiplier. Batching is worth exactly as much as deleting.

Legs 6–13 are 8 sequential round trips that could be 2–3. That is ~5–6 legs off 23.4 — roughly a quarter of the whole request — with no cache, no invalidation contract, and no staleness. Your four-axis argument for B holds up under that number: a batched read returns the same rows or fails loudly; a cache returns stale grants silently.

Q2 — localization's three identical sys_setting reads: A, filed as #10826

Agreed, with one calibration you could not have had: cloud#1539 measured those three reads as running in parallel — 3 queries but 1 leg. So the win is ~2 queries and ≈0 legs, which by the L-is-the-multiplier finding makes it low leverage on latency, not the middling item the card's ordering implies.

It is still right to do, for the reasons you give: it is the same class as the two fixes here, it is independent of whether direction 5 ever ships, and it shrinks what direction 5 would have to cache. Filed at that priority rather than as a headline.

Reporting it instead of filing it yourself was the correct call — the tranche split is the PM's, and pre-empting it is how a queue silently acquires work nobody triaged.

On this PR

Shipping 2 of 3 with the third measured out rather than quietly dropped is the right outcome. The honesty in the body — "~1/12 of a request's queries" — is what keeps a green tranche-1 PR from being read as "the performance problem is fixed". It is not; the remaining ~22 are tranche 2 and placement (cloud#1546, in flight).

The ablation discipline is the part I would hold up as the standard: three legs, each mutation confirmed on disk by grepping both the injected marker and the deleted text rather than trusting an editor's exit code, and reporting that leg B's deleted-text count read 1 instead of 0 because writeEpoch++ also occurs at the metadata-watch site. Noticing that and explaining it, rather than rounding it to "mutation applied", is the difference between an ablation and a ritual.

Mark ready when CI is green and I will enqueue.

@os-zhuang
os-zhuang marked this pull request as ready for review August 21, 2026 13:19
@os-zhuang
os-zhuang added this pull request to the merge queueAug 21, 2026
Merged via the queue into main with commit 5886ee6Aug 21, 2026
32 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-10757-dedupe-per-request-queries branch August 21, 2026 14:00
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-zhuang@hotlong