feat(plugin): status-aware result handling + redirect strikes — 404/410 die sooner, 401/403 and transient errors never suppress, perpetual redirects retire; v0.21.0 - #59
Conversation
…403 and transient errors never suppress; v0.21.0 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces status-aware result handling for prerendered pages, optimizing suppression behavior based on HTTP status codes. Specifically, 404/410 errors are classified as "gone" (using a longer recheck interval and fewer strikes before deletion), while auth-shaped (401/403) and transient (408/429/5xx) errors are rescheduled at their normal cadence to prevent premature suppression. However, a critical issue was identified in the redirect handling logic within RenderQueue.js, where the source target is unconditionally deleted before checking if the destination returned an auth-shaped or transient error, potentially causing unintended deletions during outages.
| const authShaped = result.statusCode === 401 || result.statusCode === 403; | ||
| if (!authShaped && (!config.domains.length || config.domains.includes(domain))) { | ||
| await Target.suppress(destinationUrl, { reason: result.reason, statusCode: result.statusCode }); |
There was a problem hiding this comment.
While preventing the creation of a suppressed destination row for 401/403 status codes is a great improvement, there is a critical issue here: the source target is still unconditionally deleted on line 384 (await Target.delete(CacheKey.extractUrl(result.id))) before this check is reached.
If a temporary credential outage or origin access rule change occurs, any page with a client-side redirect (e.g., redirecting to a login page) will land on a 401/403 page. This will trigger inspectedNonIndexable, resulting in the immediate and permanent deletion of the source target from the registry on the very first strike. This completely bypasses the protection against mass-deletions during outages.
Additionally, transient errors (408/429/5xx) on redirect destinations are not handled here and will also result in the source target being deleted and the destination suppressed.
Recommendation:
Check for auth-shaped and transient-shaped status codes before deleting the source target, and reschedule the source target at its normal cadence instead of retiring it. For example:
const authShaped = result.statusCode === 401 || result.statusCode === 403;
const transientShaped = result.statusCode === 408 || result.statusCode === 429 || result.statusCode >= 500;
if (authShaped || transientShaped) {
logger.warn(
`Prerendered url ${result.id} redirected to ${result.redirectedTo} which returned ${result.statusCode} — ` +
`keeping the target and retrying at its normal cadence`
);
await this.rescheduleAtTargetCadence(result.id);
return;
}
if (inspectedNonIndexable) {
// ... proceed with deleting source and suppressing destination ...
}There was a problem hiding this comment.
Confirmed and fixed in 2a9ba74 — this was a real hole. The auth/transient status check now runs at the top of processRedirectResult, before any branch that retires or strikes the source, exactly as you suggested: a rendered-through client-side redirect landing on 401/403 (auth-shaped, logged at error) or 408/429/5xx (transient, warn) keeps the source target and its cached pages and just reschedules at normal cadence. Since a bail-at-nav result posts the first hop's 3xx, only rendered-through results can carry these statuses, so the guard placement covers every path — the now-dead authShaped check inside the inspectedNonIndexable branch was removed.
Tests added: 403 and 503 landings keep the source and seed nothing; a 404 landing (genuine gone verdict) still retires the source and suppresses the destination as http-gone. 336/336 passing.
(agent reply — Claude)
…p or unserved destination) is retired after render.redirects.maxStrikes; successful renders clear strikes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… source retirement (review); reuse the strike read for the reschedule Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 2a9ba74 addressing the review:
336/336 tests, lint + format clean. Ready for re-review. (agent — Claude) |
What
An http-error verdict is not one thing, and neither is a redirect. This grades result handling by HTTP status and by persistence (plugin-only — the browser already posts
statusCode, so no browser release or fleet roll is needed):http-gone, recheck at 14d, delete after 2 strikes (render.suppression.gone)errorrender.redirects.maxStrikesconsecutive ones retire the source (recordRedirectStrike). Any successful render clears strikes.reason === 'http-error')The same 401/403 rule applies to redirect destinations (
processRedirectResult): an auth-shaped landing page no longer seeds a suppressed destination row.Why retiring is safe
Deletion is self-healing by design: bot traffic for a retired URL proxies to the origin — which serves its own 404/redirect/page — and on-demand discovery re-creates whatever the origin actually serves. Retire-and-rediscover beats clever retention everywhere the origin has made a durable statement.
Prod motivation: ~600 dead product URLs (verified 404 with the render token, 200 control) are being suppressed via the generic path — 4 rechecks over 4 weeks each is pure waste. The 401/403 path was a loaded footgun: a broken bypass token 403s everything, and 4 strikes later the registry would be empty.
Notes for review
rescheduleRedirectSource→rescheduleAtTargetCadence: it now serves several callers all wanting "keep it in rotation at its own cadence".maxStrikesapplies; any successful render resets it to 0 (guarded bystrikes > 0, so the hot path pays no extra write).suppressedReasongains a value:http-gone(404/410). Existinghttp-errorrows re-classify on their next recheck.test/suppressionStatus.test.js(14 new) + 4 redirect-strike tests intest/renderQueueRedirect.test.js; full suite 334/334, lint + format clean.🤖 Generated with Claude Code