fix(aggregator): wrap syncGetRecord errors in the CORS + no-store envelope by ascorbic · Pull Request #2167 · emdash-cms/emdash

ascorbic · 2026-07-21T07:08:31Z

What does this PR do?

Closes a pre-existing error-handling gap in the aggregator that the review of #2117 surfaced but scoped out. In apps/aggregator/src/routes/xrpc/router.ts, the syncGetRecord branch had no try/catch, so an unexpected throw (e.g. a D1 failure in fetchRecordBlob → selectBlob) escaped handleXrpc and the Worker fetch handler unwrapped — hitting workerd's bare 500 with no CORS and no cache headers, violating the endpoint's response invariant the way #2117 fixed for the policy-resolution path.

The subtlety vs #2117: syncGetRecord serves immutable record bytes with a cacheable public, max-age=300 200. So an error must not be cacheable — an unexpected throw now returns an opaque 500 carrying CORS + private, no-store (never the cacheable header), with the internal detail logged and no error.message/SQL/stack leaked. The success 200 keeps its cacheable header; 404/not-found semantics (returned as JSON inside syncGetRecord, never thrown) are unaffected.

To keep the two error paths from drifting, this extracts #2117's inlined no-store+CORS wrapper into a shared wrapDispatchError(err, context) (reusing the existing internalErrorResponse rather than duplicating it) and threads a context string so operator logs read xrpc sync.getRecord failed vs xrpc policy resolution failed instead of misattributing a blob error to policy resolution. The policy path's behavior is identical; only its log string is parameterized.

Targets the feat/plugin-registry-labelling-service integration branch. Part of #1909; a follow-up to #2117.

Type of change

Checklist

I have read CONTRIBUTING.md
pnpm typecheck passes (aggregator: clean)
pnpm lint passes (0 diagnostics)
pnpm test passes (aggregator: 342 passed / 17 files) — TDD: the new test drops a table to force a D1 error and asserts a 500 with private, no-store + CORS + no leaked internals; it fails pre-fix with the bare D1_ERROR escaping SELF.fetch. The existing success test covers the 200 + public, max-age=300 path.
pnpm format has been run
User-visible strings in the admin UI are wrapped for translation (if applicable) — n/a: server-side.
I have added a changeset (if this PR changes a published package) — n/a: @emdash-cms/aggregator is private.
New features link to an approved Discussion — n/a: bug fix; umbrella Plugin registry labelling service #1909 tracks RFC RFC: Decentralized Plugin Registry #694.

AI-generated code disclosure

This PR includes AI-generated code — model/tool: Claude Opus 4.8 (implementation), orchestrated/reviewed by Claude Opus 4.8

Screenshots / test output

aggregator: 342 passed (17 files)
pre-fix repro: D1_ERROR: no such table: publishers thrown from SELF.fetch
               (selectBlob → syncGetRecord → handleXrpc → Object.fetch), request rejected instead of wrapped

Try this PR

Open a fresh playground →

A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.

_{Tracks fix/aggregator-syncgetrecord. Updated automatically when the playground redeploys.}

…elope An unexpected error (e.g. a D1 failure in the record-blob fetch) escaped handleXrpc unwrapped, hitting workerd's bare 500 with no CORS or cache headers — the parallel gap #2117 left out of scope on the cacheable record-blob path. Extract the shared no-store+CORS wrapper (wrapDispatchError) so the sync and policy error paths stay in lockstep, add a context string for correct log attribution, and route syncGetRecord throws through it. The success 200 keeps its cacheable header; only errors get private, no-store.

changeset-bot · 2026-07-21T07:08:41Z

⚠️ No Changeset found

Latest commit: e3ee15d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

cloudflare-workers-and-pages · 2026-07-21T07:09:52Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	emdash-demo-do	`e3ee15d`	Jul 21 2026, 07:20 AM

cloudflare-workers-and-pages · 2026-07-21T07:10:14Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	emdash-demo-cache	`e3ee15d`	Jul 21 2026, 07:18 AM

cloudflare-workers-and-pages · 2026-07-21T07:10:58Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	emdash-playground	`e3ee15d`	Jul 21 2026, 07:19 AM

emdashbot

Approach judgment: This is the right fix for the right gap. syncGetRecord previously had no try/catch, so a D1 or other unexpected throw would bypass handleXrpc's envelope and surface as a bare workerd 500 without CORS or Cache-Control. Since the success path returns public, max-age=300, caching a 500 would be especially bad. Extracting the CORS + no-store wrapper into wrapDispatchError and threading a context string is a clean, in-scope refactor: it keeps the policy and sync.getRecord error paths consistent and makes operator logs correctly attribute failures instead of blaming every blob error on "policy resolution".

What I checked:

Read the diff, the full apps/aggregator/src/routes/xrpc/router.ts, and apps/aggregator/src/routes/xrpc/sync-get-record.ts.
Verified internalErrorResponse now has one caller and wrapDispatchError is only used inside handleXrpc.
Confirmed only the syncGetRecord catch block and the existing resolveRequestLabelerPolicy catch block use the new wrapper.
Traced the response clone/header flow: error responses carry private, no-store and CORS, while the success 200 keeps public, max-age=300.
Checked the new test against the schema fixtures — publishers and labelers currently have no indexes/triggers, so the DROP TABLE + sql restore pattern does not drop secondary objects. The test pattern matches the pre-existing policy-failure test.

Headline conclusion: The implementation is correct and the new regression test exercises the previously exposed failure path. No blockers.

emdashbot · 2026-07-21T07:12:09Z

+			// The success path serves a cacheable 200; an unexpected throw (e.g. a
+			// D1 error fetching the CAR blob) must fail closed with `no-store` so
+			// the error never inherits that public cache header.
+			return wrapDispatchError(err, "sync.getRecord");


[suggestion] The catch block contains a rationale comment that duplicates the wrapDispatchError docstring (lines 93–99). Per the repo’s comment-discipline conventions, comments that justify the decision (“must fail closed … so … never inherits”) should usually be deleted once the helper name and its own docstring make the intent clear.

Suggested change

// The success path serves a cacheable 200; an unexpected throw (e.g. a

// D1 error fetching the CAR blob) must fail closed with `no-store` so

// the error never inherits that public cache header.

return wrapDispatchError(err, "sync.getRecord");

} catch (err) {

return wrapDispatchError(err, "sync.getRecord");

}

Done — removed the redundant catch comment; the no-store/cache rationale lives in the wrapDispatchError docstring.

~ 🤖 Claude Fable 5

…string covers it)

github-actions Bot added the size/M label Jul 21, 2026

github-actions Bot added the review/needs-review No maintainer or bot review yet label Jul 21, 2026

github-actions Bot added the cla: signed label Jul 21, 2026

emdashbot Bot reviewed Jul 21, 2026

View reviewed changes

github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-review No maintainer or bot review yet labels Jul 21, 2026

docs(aggregator): drop redundant catch comment (wrapDispatchError doc…

e3ee15d

…string covers it)

github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Jul 21, 2026

ascorbic merged commit 9596a04 into feat/plugin-registry-labelling-service Jul 21, 2026
12 checks passed

ascorbic deleted the fix/aggregator-syncgetrecord branch July 21, 2026 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(aggregator): wrap syncGetRecord errors in the CORS + no-store envelope#2167

fix(aggregator): wrap syncGetRecord errors in the CORS + no-store envelope#2167
ascorbic merged 2 commits into
feat/plugin-registry-labelling-servicefrom
fix/aggregator-syncgetrecord

ascorbic commented Jul 21, 2026 •

edited by github-actions Bot

Loading

Uh oh!

changeset-bot Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

emdashbot Bot left a comment

Uh oh!

emdashbot Bot Jul 21, 2026

Uh oh!

ascorbic Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ascorbic commented Jul 21, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

Checklist

AI-generated code disclosure

Screenshots / test output

Try this PR

Uh oh!

changeset-bot Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Workers

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Workers

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Workers

Uh oh!

emdashbot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

emdashbot Bot Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

ascorbic Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ascorbic commented Jul 21, 2026 •

edited by github-actions Bot

Loading

changeset-bot Bot commented Jul 21, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Jul 21, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Jul 21, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Jul 21, 2026 •

edited

Loading