fix(taxonomies): never query a declared collection whose ec_* table is missing#2043
fix(taxonomies): never query a declared collection whose ec_* table is missing#2043swissky wants to merge 3 commits into
Conversation
… of probing missing ones The migration-seeded category/tag defs declare ["posts"] whether or not that collection exists, so on any site without a posts collection every visible-term-count computation issued a UNION that joined ec_posts and failed. The retry path made the result correct, but the failed statement itself is logged by the database — on D1 it surfaces as a "no such table: ec_posts" error span in Workers Observability, once per taxonomy per uncached render, drowning out real errors. Resolve the set of existing ec_* tables upfront (cached per isolate, reset by SchemaRegistry create/delete and by readers that observe a stale list) and scope the count to collections whose table exists, so a query referencing a missing table is never sent.
🦋 Changeset detectedLatest commit: 05ed138 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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 |
github-actions
Bot
added
review/needs-review
@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: |
github-actions
Bot
added
the
query-count changed
Query-count snapshot changes4 routes changed, total Δ +4 queries. SQLite
D1
Comparing snapshot files between base and head. Updated automatically on each push. |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Is it possible to do this without adding an extra query to the hot path?
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a sound, well-scoped bug fix. The problem is real and clearly diagnosed: migration 006 seeds category/tag bound to ["posts"], so any site without a posts collection was hitting a missing-table error on every uncached term-count render. Moving from "query optimistically and retry" to "resolve existing ec_* tables up front" eliminates the phantom D1/SQLite error spans, and the per-isolate cache matches the existing taxonomy-definitions cache pattern. Invalidation is wired into SchemaRegistry.createCollection and deleteCollection, which are the only production paths that create or drop content tables (seeds, admin API, MCP, and WP import all go through the registry).
I checked the changed files, traced call sites, verified listTablesLike handles both SQLite/D1 and Postgres safely, and confirmed that the query-count snapshots and test DB cache reset are consistent. The changeset is present and correctly targeted at the emdash package. No AGENTS.md convention violations (i18n, API envelope, locale filtering, SQL safety, index discipline) were introduced.
One issue: the stale-cache fallback in fetchVisibleTermCounts refreshes the table list and retries once, but the final runCounts call is unprotected. If another isolate drops a table between the fresh lookup and the retry, the function still throws a missing-table error instead of returning the partial counts it used to. Fixing that preserves both the primary goal (no failed statements in the steady state) and the old partial-but-correct recovery behavior in the race case.
| resetContentTableNamesCache(); | ||
| const fresh = await getContentTableNames(db); | ||
| const remaining = present.filter((collection) => fresh.has(`ec_${collection}`)); | ||
| if (remaining.length === 0) return new Map(); | ||
| return runCounts(db, taxonomyName, remaining); |
There was a problem hiding this comment.
[needs fixing] The stale-cache fallback refreshes getContentTableNames and filters to remaining, but the final return runCounts(...) is unprotected. If a collection is dropped by another isolate after the fresh lookup but before the retry executes, this still throws a missing-table error and surfaces the exact kind of phantom DB error the PR is eliminating. The original per-collection loop skipped individual missing tables and returned partial counts; the retry path should do the same.
| resetContentTableNamesCache(); | |
| const fresh = await getContentTableNames(db); | |
| const remaining = present.filter((collection) => fresh.has(`ec_${collection}`)); | |
| if (remaining.length === 0) return new Map(); | |
| return runCounts(db, taxonomyName, remaining); | |
| resetContentTableNamesCache(); | |
| const fresh = await getContentTableNames(db); | |
| const remaining = present.filter((collection) => fresh.has(`ec_${collection}`)); | |
| if (remaining.length === 0) return new Map(); | |
| // A table could still be dropped between the fresh lookup and the count | |
| // query (another concurrent isolate). Count each remaining collection | |
| // individually so a missing table is skipped rather than failing the request. | |
| const counts = new Map<string, number>(); | |
| for (const collection of remaining) { | |
| try { | |
| for (const [group, count] of await runCounts(db, taxonomyName, [collection])) { | |
| counts.set(group, (counts.get(group) ?? 0) + count); | |
| } | |
| } catch (error) { | |
| if (!isMissingTableError(error)) throw error; | |
| } | |
| } | |
| return counts; | |
| } |
There was a problem hiding this comment.
Done — the retry now counts each remaining collection individually inside its own try/catch and skips missing tables, matching the original per-collection partial-count behavior instead of re-throwing on a concurrent drop.
github-actions
Bot
added
review/awaiting-author
The stale-cache retry re-ran the whole batch, so a table dropped between the fresh lookup and the retry still threw. Count each remaining collection individually and skip missing tables, matching the original per-collection skip behavior (emdashbot review).
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a sound, well-scoped bug fix, and the issue raised in the prior review has been addressed.
Approach judgment: The problem is real and the fix is the right shape. Default taxonomy definitions are seeded bound to posts, so sites without a posts collection previously hit a missing-table error on every uncached term-count render. Moving from "query optimistically and retry" to "resolve existing ec_* tables upfront" eliminates the phantom D1/SQLite error spans, and the per-isolate cache follows the same globalThis/Symbol pattern already used for taxonomy definitions. Invalidation is wired into SchemaRegistry.createCollection and deleteCollection, the production paths that create or drop content tables.
What I checked:
fetchVisibleTermCountsnow resolves the table set once per isolate, skips missing tables without sending a failing statement, and refreshes the cache if it discovers a stale entry.- The stale-cache fallback now iterates per remaining collection with a missing-table guard, so a table dropped between the fresh lookup and the retry is skipped rather than failing the request — this directly fixes the prior review finding.
content-tables-cache.tsuses the sameglobalThis+Symbol.forsingleton pattern as the existing taxonomy-definitions cache and correctly bypasses the cache for isolated DBs.- SQL is safe: collection slugs are validated before building
ec_${slug}, identifiers usesql.ref, andlistTablesLikeis parameterized for both SQLite and Postgres. - The query-count snapshots correctly show the one-off cold-isolate table lookup on term-page routes and no extra query on warm renders.
- Tests cover the no-failed-statement guarantee and the stale-cache recovery path;
test-db.tsresets the isolate cache when creating a fresh test database. - Changeset is present and user-facing.
No AGENTS.md violations (i18n, API envelope, auth, locale filtering, SQL safety, index discipline, changeset) were introduced. I did not run the test suite, linter, or builds; I reviewed statically as instructed, and the PR author reports the full test suite passes.
github-actions
Bot
added
review/approved
There was a problem hiding this comment.
I've been looking a bit more deeply into how this could avoid the extra query, and found another issue. The per-isolate table cache introduces a correctness problem after collection creation. If isolate A has cached the table set and isolate B creates a collection, A never refreshes: unlike deletion, creation produces no missing-table error to trigger recovery. A taxonomy that already declares that collection (notably the migration-seeded posts defaults) will therefore undercount on A until the isolate is recycled. Resetting the cache in SchemaRegistry.createCollection() only fixes the isolate that handled the write.
Can we avoid adding this independent, permanently stale schema cache? Runtime initialisation already queries _emdash_collections for the auto-seed gate. One approach would be to select the registered slugs instead of only COUNT(*), derive the count from those rows, and populate an invalidation-aware collection cache that the taxonomy-definition read can use. handleTaxonomyList() already treats _emdash_collections as the filter for orphan references. Collection create/delete should compensate partial D1 operations so the registry/table invariant remains reliable. Contexts that do not perform that initialization could retain an explicit fallback.
This would also avoid the extra cold-isolate query on the normal path. If physical-table discovery remains necessary, it at least needs bounded or cross-isolate revalidation so newly created collections become visible without waiting for isolate eviction.
github-actions
Bot
added
review/awaiting-author
2 participants
What does this PR do?
Stops term counting from issuing queries against
ec_*tables that don't exist, which currently floods D1 logs with phantomno such table: ec_postserrors on any site without apostscollection.The bug: migration
006_taxonomy_defsseeds the defaultcategory/tagdefs bound to["posts"]whether or not that collection is ever created.fetchVisibleTermCountsthen builds a UNION that joinsec_posts, catches the missing-table error, and retries per collection — the result is correct, but the failed statement itself is logged by the database. On D1 each one surfaces as ano such table: ec_posts: SQLITE_ERRORerror span in Workers Observability, once per taxonomy per uncached render, drowning out real errors. (Observed in production on a Workers + D1 site with Logs/Traces enabled; the same drift also occurs when a declared collection is deleted.)The fix: resolve the set of existing
ec_*tables upfront (via the dialect-awarelistTablesLike, same helper migration 051 uses) and scope the count to collections whose table actually exists — a failing statement is never sent. To keep the count path at one round-trip per taxonomy, the table list is cached per isolate in a newdatabase/content-tables-cache.ts(same pattern as the loader's taxonomy-names cache, including thedbIsIsolatedbypass) and reset bySchemaRegistry.createCollection/deleteCollection. A reader that still hits a dropped table (stale list from another isolate's delete) resets the cache and retries once, preserving the previous partial-but-correct behavior.The
getTermquery-budget test stays at 4 queries; it now primes the once-per-isolate table lookup so the budget reflects the steady state.Tests:
never sends a query referencing a missing ec_* table— asserts on the SQL sent through the Kysely executor; fails onmain, passes with the fix.recovers when the isolate-cached table list goes stale— covers the refresh-and-retry fallback.Possible follow-up (out of scope here): stop hardcoding
["posts"]in the migration-seeded defaults and let seeds declare the binding — happy to open a Discussion if that's of interest.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.AI-generated code disclosure
Screenshots / test output
Production log entry that motivated the fix (Workers Observability, one per taxonomy per uncached render):