fix: skip byline query path when the bylines table is empty - #2304
fix: skip byline query path when the bylines table is empty#2304edrpls wants to merge 3 commits into
Conversation
The folded-byline fast path fell back to the full query path whenever any entry in the batch had an author_id but no folded credits. On sites that never use bylines every entry matches that shape, so every content read paid getBylinesForEntries round trips (chunked content-byline lookups plus a user-id lookup per locale bucket) that could only return zero rows. Fold an uncorrelated existence probe on _emdash_bylines into the content query (evaluated once per statement — no extra round trip). An empty table makes an empty fold authoritative: no credit can exist in any locale and the author fallback has no byline to resolve to, so hydration serves the folded result directly. When the probe is missing (cached snapshots) or the table has rows, the conservative fallback is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 63c1a4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR addresses a real, measured performance problem: sites that never create bylines were paying getBylinesForEntries lookups on every content read because the fast-path fold was conservatively discarded for any authored entry. Folding an existence probe into the content SELECT so the empty-bylines case can trust the empty fold is a clever, stateless fix that avoids caching/invalidation complexity, and the tests drive the real SQLite loader with query-count assertions covering both the short-circuit and the non-short-circuit case.
I read the diff, full loader.ts/query.ts, the new test, and AGENTS.md. The logic is correct: the marker is set only when the folded column is present, missing markers are treated as "unknown" (safe fallback), and the author-fallback still falls through to the query path when bylines exist. The new folded column is also correctly excluded from data via SYSTEM_COLUMNS.
The one convention issue is that the code adds a SELECT 1 FROM _emdash_bylines LIMIT 1 existence probe. AGENTS.md is explicit: "Don't add a SELECT id FROM foo LIMIT 1 to skip work on empty sites — on live sites you pay the extra query every request for no gain." That rule does not currently carve out probes folded into the same statement. The author makes a reasonable case that this is zero extra round trips and was measured to help production deployments, but it is still the pattern the guideline prohibits. This needs either an AGENTS.md amendment or an alternative approach; until that is resolved I can't call it clean.
| // 1 when `_emdash_bylines` has any row, NULL when empty. An empty table | ||
| // means an empty fold is authoritative — no credit in any locale, no | ||
| // author-fallback byline — so hydration can skip the byline query path. | ||
| const bylinesExist = sql`(SELECT 1 FROM ${sql.ref("_emdash_bylines")} LIMIT 1) AS ${sql.ref("_emdash_bylines_exist")}`; |
There was a problem hiding this comment.
[needs fixing] Adds (SELECT 1 FROM _emdash_bylines LIMIT 1) as a folded existence probe. AGENTS.md states: "Don't add a SELECT id FROM foo LIMIT 1 to skip work on empty sites — on live sites you pay the extra query every request for no gain." This pattern matches that rule verbatim. While the PR reasonably folds it into the existing SELECT to avoid an extra round trip, the guideline as written prohibits any such probe. Please either update AGENTS.md to explicitly permit same-statement existence probes when they reduce total queries, or find a different mechanism (for example, a cheap per-epoch/bylines count cached at the loader level) that does not add a LIMIT 1 probe to every logged-out content fetch.
There was a problem hiding this comment.
Fair reading of the rule as written. I've amended AGENTS.md in this PR to carve out the same-statement shape explicitly: the rule's cost rationale (an extra round trip on every live request) doesn't apply to an uncorrelated scalar subquery folded into a select list the request already runs — SQLite and Postgres both evaluate it once per statement. The CI query-count snapshots on this PR confirm the shape: the recorded query texts changed, the per-route counts did not.
The alternative (a per-epoch cached count) was considered and rejected: it pays a real per-request options read on the query path and needs cross-isolate invalidation wiring on byline creation, which is strictly worse than a zero-round-trip probe with no invalidation at all. Happy to adjust the AGENTS.md wording if maintainers prefer different phrasing.
github-actions
Bot
added
review/needs-rereview
The rule exists because a separate LIMIT 1 round trip costs every live request. A probe folded into a query the request already runs has no such cost, and the byline hydration fast path relies on exactly that shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d99abe7 to
63c1a4e
Compare
1 participant
What does this PR do?
Fixes sites that never use bylines paying dead byline lookups on every content read.
hydrateEntryBylinesfalls back from the folded fast path to the full query path when any entry in the batch has anauthor_idbut no folded credits (a batch-widesome()). That conservatism exists for a real reason — a credit in a different locale is invisible to the locale-correlated fold — but on a site whose_emdash_bylinestable is empty, every authored entry matches that shape, so the fast path is effectively always dead. Every logged-out render paysgetBylinesForEntries: a chunked_emdash_content_bylineslookup plus a_emdash_bylines-by-user_idlookup per locale bucket, all of which can only return zero rows.Measured on a production deployment (Macabro festival site, emdash 0.31.1, 2,143 entries): these dead lookups were 25–45% of the emdash queries on every page render.
The fix folds an uncorrelated existence probe —
(SELECT 1 FROM _emdash_bylines LIMIT 1)— into the content query alongside the existing folded byline subquery. Uncorrelated scalar subqueries are evaluated once per statement on both SQLite and Postgres, so this adds zero round trips and no per-row cost (per the "logged-out hot path only ratchets down" rule; no caching or invalidation is involved because the signal rides the query it protects). An empty table makes an empty fold authoritative: no credit can exist in any locale, and the author fallback has no byline to resolve to. When the probe is missing (e.g. cached snapshots) or the table has rows, behavior is unchanged — including the deliberate locale-mismatch fallback.Two tests drive the real loader (mocked
astro:contentdelegating toemdashLoader()) on a real SQLite database with a query-countingloghook:bylines: []/byline: nullwith zero byline-table queries (fails onmainwith the two dead lookups);Found during a measured database audit of a production deployment. Note for CI: the query-count snapshot diff for logged-out routes should only go down or stay flat with this change.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. — n/a: no admin UI strings changedAI-generated code disclosure
Screenshots / test output
Failing first (on
main, before the fix) — the two dead lookups issued against an empty bylines table:After the fix, full
packages/coresuite:The single failure (
virtual-modules.test.ts › watches resolved sandbox plugin entries) is pre-existing on a cleanmaincheckout in this environment (macOS/varvs/private/vartemp-dir realpath mismatch) and unrelated to this change.🤖 Generated with Claude Code