fix: attribute redirect-driven content misses to the requested path in the 404 log - #2305
fix: attribute redirect-driven content misses to the requested path in the 404 log#2305edrpls wants to merge 3 commits into
Conversation
The redirect middleware logs any 404 response keyed by pathname, so hits
on the site's own /404 route were recorded as misses. emdash's content
templates answer a miss with Astro.redirect("/404"): the real path is
logged on the first pass, the browser follows the redirect, and the /404
render logs a second row — every real miss counted twice, with the
missed-path signal buried under one giant "/404" entry (41% of 55k rows
on an audited production log).
Skip logging when the pathname is /404 (or /404/ for trailing-slash
sites). The error route returns 404 by design on every visit, so a row
for it carries no information, and the redirect flow already captured
the real missed path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 4378733 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: |
github-actions
Bot
added
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for a real production issue: EmDash templates answer content misses with Astro.redirect("/404"), so Astro's /404 route renders with status 404 on the follow-up request and was being logged as a missed path, inflating the 404 summary. Filtering on pathname is the correct signal — filtering by routePattern would discard the genuinely valuable first-pass rows.
What I checked:
- The diff, the full middleware, the redirect repository, the 404-log migration (035), and the test file.
- Query-impact: the change is a pathname string comparison; it adds no DB round-trips on the logged-out hot path.
- Test quality: a failing regression test is added and the assertions exercise observable behavior (no
log404call, empty DB table). - Conventions: changeset is present and user-facing; no new admin UI strings; no SQL interpolation issues.
The implementation is sound. I have two non-blocking suggestions: the new comment is verbose and contains narrative/justification that AGENTS.md discourages, and the exclusion is so narrow that it misses Astro-localized error pages (/fr/404, /es/404) in EmDash's i18n-routed fixtures/templates. The latter doesn't affect the reported redirect-loop bug (templates redirect to the absolute /404), but it's worth recording if direct visits to localized 404 pages should also be suppressed.
| // Log 404s for unmatched paths (fire-and-forget). The site's own error | ||
| // page is excluded: its route answers 404 on every render — including | ||
| // the follow-up request after a content miss redirects to /404 — so | ||
| // logging it counts every real miss twice under one meaningless | ||
| // "/404" row while the missed path is already logged on the first pass. |
There was a problem hiding this comment.
[suggestion] The comment explains a non-obvious "why" (the /404 route answers 404 by design), but it also restates the bug narrative from the PR description and justifies the fix with phrases like "meaningless" and "already logged on the first pass". Per AGENTS.md, comments should not be PR summaries or justification; they should give future readers only what the code can't say itself.
| // Log 404s for unmatched paths (fire-and-forget). The site's own error | |
| // page is excluded: its route answers 404 on every render — including | |
| // the follow-up request after a content miss redirects to /404 — so | |
| // logging it counts every real miss twice under one meaningless | |
| // "/404" row while the missed path is already logged on the first pass. | |
| // Log 404s for unmatched paths (fire-and-forget). Skip /404 itself — Astro | |
| // serves the site error page there with status 404 on every render, so logging | |
| // it would double-count the original miss after templates redirect here. |
There was a problem hiding this comment.
Adopted your suggested wording verbatim — pushed in a636351.
| // the follow-up request after a content miss redirects to /404 — so | ||
| // logging it counts every real miss twice under one meaningless | ||
| // "/404" row while the missed path is already logged on the first pass. | ||
| if (response.status === 404 && pathname !== "/404" && pathname !== "/404/") { |
There was a problem hiding this comment.
[suggestion] The exclusion only handles /404 and /404/. EmDash's fixtures and demos configure Astro i18n routing with locales: ["en", "fr", "es"] and defaultLocale: "en", so Astro also generates locale-prefixed error pages at /fr/404 and /es/404. A direct visit to one of those still gets logged, which is inconsistent with the rationale for skipping the error page.
This does not affect the reported bug because templates redirect to the absolute path /404, but it's a gap worth considering. If you keep the current scope, consider adding a short comment or test noting the limitation.
There was a problem hiding this comment.
Agreed it's a real gap, and deliberately left out of this PR: excluding locale-prefixed error pages needs the configured locale list (the middleware currently does zero i18n/base handling anywhere, so an exact-path check keeps parity with the rest of the file), and a locale-shaped regex would guess. The reported amplification only involves the absolute /404 — templates redirect there regardless of locale. If maintainers want the localized variants excluded too, I'd wire getI18nConfig() into the middleware in a follow-up rather than widen this diff.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
The template pattern for a content miss is Astro.redirect("/404"): the
first pass answers 302 — which the 404 logger never saw — and only the
follow-up /404 render logged, as one aggregate "/404" row. The real
missed path was never recorded, and excluding /404 alone would have
removed the only signal those misses had.
Log the miss on the first pass instead, keyed by the requested path,
when a route answers with a redirect whose target is the error page.
Direct hits on /404 stay unlogged — the route answers 404 by design and
carries no path information.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
edrpls
changed the title
|
Heads-up: this PR changed shape in 10d5bce — the original fix was wrong in a way worth being explicit about. The first revision excluded The current shape closes the underlying hole instead: a redirect whose target is the error page is logged as a miss under the requested path on the first pass, and |
10d5bce to
4378733
Compare
1 participant
What does this PR do?
Fixes 404 logging for content misses — both the noise and a telemetry hole the noise was hiding.
The documented template pattern answers a content miss with
return Astro.redirect("/404")(every demo and the site-building docs use it). That flow has two consequences for the 404 log:response.status === 404) never saw — so the real missed path was never recorded./404, which Astro serves with status 404, and that got logged — so every content miss collapsed into one aggregate"/404"row. On the audited production deployment (Macabro festival site, emdash 0.31.1), that row carried 41% of 55k logged hits, burying the per-path signal while recording no usable path itself.The fix logs the miss on the first pass, under the path the visitor actually requested: when a matched route answers with a redirect whose target is the error page (
/404or/404/), that request is a content miss and is logged as such. Direct renders of/404itself are never logged — the route answers 404 by design and carries no path information. Fully-unmatched paths (bot scans, broken links to non-routes) keep logging exactly as before, since Astro renders the error page for them on the first pass with the real pathname.Net effect for upgraders: the meaningless aggregate
/404row stops growing, and the log gains something it never had — the actual missed paths for content misses. Ordinary redirects (not targeting the error page) are not logged.An earlier revision of this PR only excluded
/404from logging, on the assumption the missed path was already recorded on the first pass. Adversarial review of the branch proved that assumption wrong for the redirect flow (the first pass is a 302), which would have silently zeroed 404 telemetry for content misses — hence the current shape, which closes the hole instead. The test suite now models the full production sequence: 302-to-/404followed by the/404render asserts exactly one row, keyed by the real path (fails onmain, which records"/404"instead), alongside tests for unmatched paths, ordinary redirects, and direct error-page visits.Known limitations, deliberately out of scope: no
base-path handling (consistent with the rest of the middleware, which matches raw pathnames everywhere), and locale-prefixed error pages (/fr/404) are not excluded for direct visits — excluding them properly needs the configured locale list, and the redirect flow is unaffected (templates redirect to the absolute/404). The pre-existing aggregate/404row on already-deployed sites is retained (deleting it would be a data-deleting migration, worth a separate explicit decision).Found during a measured database audit of a production deployment.
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
The redirect-flow regression test fails on
main(the miss is recorded as"/404"instead of the real path):After the fix — middleware suite plus redirect integration suites:
🤖 Generated with Claude Code