fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop

[FagnerMartinsBrack]

fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop

Hypothesis

Two visibility holes hide save-pipeline pain: (1) view_save_intent has an outcome="error" enum value that is never emitted for the failures users actually hit (URL validation 422s), so the save funnel reads as if saves never fail; (2) the generate-summary worker treats "canonical content absent because the crawl terminally failed" identically to "content not yet written", producing a permanent retry/DLQ loop via the summary auto-heal that fires almost daily and drowns the errors dashboard.

Evidence (30d, Jun 4 – Jul 4 2026)

• view_save_intent outcome="error": 0 events in 30d — yet the enum exists and the catch-path emits are wired. Every validateSaveableUrl 422 branch (queue save bar, extension save-article/save-html/save-content) renders a user-facing save error without emitting.
• 170 generate-summary DLQ invocations, sole error AssertionError "Article content not found: <url>", ~520 retry ERROR lines, near-daily. Mechanism (verified in code): markCrawlExhausted leaves summary=failed(crawl-failed) → stale-check auto-heal reprimes any summary=failed row (3 attempts, then one round per 24h forever) → GenerateSummary finds no canonical content (crawl is terminal) → assert → 4 receives → DLQ → markSummaryExhausted → failed(exhausted-retries) → eligible again next tick. The 73 crawl-failed dead-lettered anonymous first-saves are the seed population.
• Duplicated reason suffix "exhausted-retries (receiveCount=4) (receiveCount=4)" (167/170 events): already fixed on main by fix(summary): stop doubling receiveCount in exhausted-retries parse-error #886 (commit 3dd818a5, Jul 3) with a regression test — intentionally not touched here.

What this changes

1. Emit view_save_intent outcome="error" on every synchronous save failure: queue save bar validation 422, extension save-article validation 422, save-html invalid-request/invalid-URL 422s, save-content missing-field/invalid-URL/unsupported-media/not-a-pdf 422s. buildSaveIntentEvent now derives article_host via a null-safe parser (article_host/content_class become nullable — dashboards already coalesce() these). Deliberately not instrumented: bulk save-all-tabs per-entry skips (expected filtering of chrome:// tabs; per-job failures already emit), multipart-parse 422s/406s (no URL available), and async crawl failures (out of scope by design).
2. New dashboard widget "Save errors by surface (view_save_intent outcome=error)" — widget count 27 → 28.
3. generate-summary handler: when content is absent AND the aggregate row's crawl.kind is "failed"/"unsupported", mark the summary skipped ("crawl-failed"/"crawl-unsupported" — the same terminal collapse markCrawlBlocked/markCrawlNotFound already write) instead of throwing. Skipped is terminal: the auto-heal gate stops repriming, the loop ends, and existing stuck rows self-drain on their next (final) auto-heal round — no migration. The genuine race (crawl=pending) and genuine inconsistency (crawl=ready, content missing) keep the assert → retry → DLQ path.

Estimated result

• Save-failure visibility goes from 0 to actuals (the 368 save-intents/30d finally get a denominator-side error count; malformed-URL friction on the queue save bar becomes measurable).
• generate-summary DLQ: ~170/mo → ~0 within 48h of deploy (one final round per stuck row); ~520 retry ERROR lines/mo eliminated; errors dashboard noise floor drops accordingly.

How to measure

Save errors (hutch handler log group):

fields @timestamp, surface, path, article_host
| filter stream = "analytics" and event = "view_save_intent" and outcome = "error"
| stats count(*) as errors by surface, path
| sort errors desc

DLQ decay (generate-summary log group):

filter @message like "Article content not found"
| stats count(*) as hits by bin(1d)

Expect the first query to go nonzero within days and the second to decay to zero within ~48h.

Conflict note

analytics-dashboard.test.ts asserts an exact widget count (27 on main at time of writing; this PR bumps it to 28) plus non-overlapping grid positions. Sibling PRs #916–#918 touching the dashboard will trivially conflict on that literal and possibly the grid row at y:122. Resolution: after rebase, re-count the widgets.push entries, set the literal to the real total, and shift this PR's widget to the next free 12-wide slot if taken.

Out of scope (deliberate)

Crawler-canary follow-up for the top DLQ'd hosts (fagnerbrack.com ×9, trends.google.com ×6, unsplash.com ×5, reddit/HN/npmjs ×2 each) is not attempted here: per CLAUDE.md's crawler-health rule, each fix starts by reproducing the failing fetch locally against the real URL and iterating until the canary passes — a separate, per-host effort. This PR only stops those already-failed rows from generating infinite summary retries. Also untouched: markCrawlExhausted's summary=failed(crawl-failed) write (converting it to skipped would disable recrawlTieKeptCanonical's recovery predicate — candidate follow-up).

https://claude.ai/code/session_01En2uUiiwQoRQKWenCnkPi9

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop #937
• Commit: 8cf9e38

[claude]

Claude finished @FagnerMartinsBrack's task in 11m 38s —— View job

---

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. The "out of scope" note's recrawlTieKeptCanonical rationale doesn't hold (but there's no regression)

The PR body says markCrawlExhausted's summary=failed(crawl-failed) write is left untouched "converting it to skipped would disable recrawlTieKeptCanonical's recovery predicate". That predicate keys on summary.kind === "failed" && reason.kind === "crawl-failed" (recrawl-tie-kept-canonical.ts:23-25). But the new handler branch flips exactly those rows to summary=skipped(crawl-failed) on the first auto-heal round (which is the intended DLQ fix), so the predicate stops matching them regardless of leaving markCrawlExhausted alone — the stated mitigation doesn't actually preserve that path.

No functional regression, though: this branch only runs when there's no canonical (!article), and tie-kept recovery only fires when canonical already exists. A no-canonical row that later gets content recovers via recrawlPromoteTier → publish-canonical-content-changed → summary re-prime, which is unconditional and summary-state-independent (recrawl-promote-tier.ts:22-29). Worth correcting the note so a future reader doesn't rely on the failed state to keep recovery alive.

2. Two outcome-agnostic dashboard widgets will now fold error events into "saves"

analytics-dashboard.ts:423 ("Save-intent by surface × content class") and :438 ("…by content class %") filter only on event = view_save_intent with no outcome filter. This PR turns outcome=error from ~0 events into a common event, so those widgets' saves totals now include validation errors, and the pie gets a first-time unclassified slice (error events carry content_class = null). It's consistent with the widgets' existing "count all intents" design (they already aggregate saved + prompted_to_sign_up), so this may be intended — but if they're meant to track successful/attempted saves, add and outcome <> "${SAVE_OUTCOMES.error}" to both. Fix this →

3. No explicit test for the preserved crawl=pending race

The new branch keeps the assert→DLQ path for the genuine race (crawl=pending, content absent). That path is covered transitively by the existing "content not found" test (generate-summary-handler.test.ts:219), but that fixture is crawl: { kind: "ready" } (line 43) — it exercises the crawl=ready inconsistency, not the crawl=pending race. A dedicated crawl=pending case would document the branch the PR explicitly calls out. Minor. Fix this →

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.

---

Review notes

Verified the two load-bearing claims:

• DLQ loop terminates. The new terminal write reaches summary=skipped, and the auto-heal gate decideSummaryAutoHeal returns "skip" for anything that isn't summary=failed (decide-summary-auto-heal.ts:25), and the handler's cache-hit check short-circuits skipped (generate-summary-handler.ts:40-44). Loop closes. The !article && crawl ∈ {failed, unsupported} guard is correctly narrow — crawl=pending (race) and crawl=ready (inconsistency) still DLQ. crawl-failed/crawl-unsupported are valid SummarySkipReasons (already written by markCrawlBlocked/markCrawlNotFound).
• Save-error emits are clean. Every new emitSaveIntent(...error) is immediately followed by return, and the catch blocks only run after the success path's pre-emit statements, so no double-emit. article_host/content_class widening to nullable is safe — no TS consumer reads them off the event, and the existing save-funnel widgets already coalesce(content_class, "unclassified"). articleHostFromSubmitted has full branch coverage; articleHostFrom still has production callers in view.page.ts (no dead export). Deliberate omissions (multipart-parse 422 at queue.page.ts:942, 406) match the PR description.

Solid, well-scoped, well-tested PR. The three notes above are all optional.

_{Note: this was a manual @claude review via the listener, so I've posted the review here in the comment (the only channel visible in this flow) rather than deferring to the auto-apply workflow.}
· fix/save-pipeline-visibility

[FagnerMartinsBrack]

⚠️ Auto-fix skipped: the review run produced no parseable review block (missing CLAUDE_REVIEW_START/END): no CLAUDE_REVIEW block found in the run output. The full review is in the run's tracking comment — address any high/medium findings manually.

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop #937
• Commit: 68bd979

[claude]

Claude finished @FagnerMartinsBrack's task in 6m 25s —— View job

---

PR Review: fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop

• Read reviewer instructions + CLAUDE.md guidelines
• Gather PR diff and commit history
• Review save-intent instrumentation (analytics, content-source, queue routes)
• Review generate-summary DLQ-loop fix
• Review dashboard widget + tests
• Post final review

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. The two outcome-agnostic save-intent widgets now fold validation errors into saves

analytics-dashboard.ts:423 ("Save-intent by surface × content class") and :438 ("…by content class %") filter only on event = view_save_intent with no outcome filter. This PR turns outcome=error from ~0 events into a common event, so both widgets' saves totals now include validation errors, and the pie gains a first-time unclassified slice (error events carry content_class = null). The query is unchanged by this PR, and counting all intents is these widgets' established design (they already aggregate saved + prompted_to_sign_up), so this is plausibly intended — but the column alias saves becomes a slight misnomer and the own-vs-third-party ratio is diluted by unclassifiable malformed-URL errors. If they should track attempted/successful saves, add and outcome <> "${SAVE_OUTCOMES.error}" to both. Fix this →

2. Drift-test annotation counts the new widget as save-funnel, but the code places it under Homepage A/B

The drift test now reads …4 save-funnel, 1 summary-engagement, 2 audience-device, 1 errors, 1 homepage-ab (analytics-dashboard.test.ts:72). But the new "Save errors by surface" widget is physically pushed inside the Homepage A/B widgets.push(...) block (analytics-dashboard.ts:586), whose section comment (:563) only describes the A/B experiment. So physically it's 3 save-funnel / 2 homepage-ab, and a future reader finds a save-errors widget under an A/B-experiment comment. The total (28) is correct and the drift test still guards it — only the human-readable section mapping (the annotation's whole purpose) is now inconsistent. Cleanest fix: move the logWidget object into the save-funnel widgets.push block (near :452), keeping its x:12, y:122 coords, so the annotation matches the code. Fix this →

3. No dedicated test for the preserved crawl=pending race

The new handler branch deliberately keeps the assert→DLQ path for the genuine race (crawl=pending, content absent). The PR adds tests for crawl=failed→skipped, crawl=unsupported→skipped, and the no-row DLQ case — good — but the crawl=pending case is still only covered transitively by the existing "content not found" fixture, which is crawl: { kind: "ready" } (generate-summary-handler.test.ts:43) — i.e. it exercises the ready inconsistency, not the pending race. A dedicated crawl=pending case would document the exact branch the PR calls out. Minor. Fix this →

4. PR-body "out of scope" rationale for recrawlTieKeptCanonical is inaccurate (no code impact)

The body says markCrawlExhausted's summary=failed(crawl-failed) write is left untouched "converting it to skipped would disable recrawlTieKeptCanonical's recovery predicate". That predicate keys on summary.reason.kind === "crawl-failed" while summary.kind === "failed" — but this PR's new branch flips those same rows to summary=skipped(crawl-failed) on the first auto-heal round anyway, so the predicate stops matching them regardless of leaving markCrawlExhausted alone. No regression (this branch runs only when !article, and tie-kept recovery fires only when canonical already exists; a later-recovered row re-primes via recrawlPromoteTier), but the stated mitigation doesn't actually preserve that path — worth correcting so a future reader doesn't rely on the failed state to keep recovery alive.

Verification notes

I independently re-verified the two load-bearing claims on commit 68bd979d:

• DLQ loop terminates, with no spurious "reader-view succeeded" side effect. The new branch calls markSummarySkipped → summary=skipped (terminal); decideSummaryAutoHeal returns skip for anything not summary=failed, and the handler's cache-hit check short-circuits skipped, so the reprime loop closes. Critically, markSummarySkipped only publishes publish-reader-view-loading-succeeded when deriveReaderViewStatus is succeeded — and for crawl ∈ {failed, unsupported} that function returns failed (article-state.ts:42), so no false success notification is emitted. crawl-failed/crawl-unsupported are valid SummarySkipReasons and match exactly what markCrawlBlocked/markCrawlNotFound already write. The !article guard makes the branch robust to a stale existing.crawl (recovered content still routes to summarize; pending race and ready inconsistency still DLQ). Loop is correctly narrow.
• Save-error emits are clean — no double-emit. Traced all four surfaces (save-article, save-html, save-content, queue/save): every new emitSaveIntent(...error) is immediately followed by return or sits in a mutually-exclusive branch; success (saved) and catch (error) paths never overlap. article_host/content_class widening to nullable is safe — grepped all .ts; the only remaining references are doc comments, no consumer reads them as non-null. articleHostFromSubmitted has full branch coverage (valid host / unparseable / empty string / empty host). New route tests correctly distinguish malformed-URL (article_host: null) from schema-valid-but-unsaveable (article_host: "localhost"/"example.com").

Deliberate omissions (multipart-parse 422, 406, bulk per-entry skips, async crawl failures) match the PR description. Widget grid: new widget at x:12, y:122 sits beside Homepage A/B (x:0, y:122) on a 24-wide grid — no overlap.

Solid, well-scoped, well-tested PR. All four notes above are optional.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.
· fix/save-pipeline-visibility

[FagnerMartinsBrack]

⚠️ Auto-fix skipped: the review run produced no parseable review block (missing CLAUDE_REVIEW_START/END): no CLAUDE_REVIEW block found in the run output. The full review is in the run's tracking comment — address any high/medium findings manually.

[FagnerMartinsBrack]

@claude Resolve conflicts. Use your Read tool to read the local file .github/workflows/claude-PR-conflict-fixer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop #937
• Branch: fix/save-pipeline-visibility

[claude]

Claude finished @FagnerMartinsBrack's task in 27m 36s —— View job

---

Conflicts resolved ✅

• Read conflict-fixer instructions + PR commit history
• Rebase onto origin/main
• Resolve conflicts (cross-referencing PR intent)
• Run pnpm check to verify
• Push with --force-with-lease

Rebased fix/save-pipeline-visibility onto origin/main and pushed (68bd979d → c29b75e2). All conflicts traced to #941 (feat(blog-analytics)), which both moved the analytics code into a new @packages/web-analytics package and added a blog-traffic dashboard widget. Three conflicts — one textual, two git-invisible semantic ones:

#	Type	Conflict	Resolution
1	Textual	analytics-dashboard.test.ts — both sides bumped the widget count 27→28, but for different widgets (main's blog-traffic vs. this PR's save-errors). Both landed, so the real total is 29.	Set toHaveLength(29) + merged the breakdown (4 save-funnel … 1 homepage-ab, 1 blog-traffic).
2	Semantic (grid)	This PR's save-errors widget and main's blog-pageviews widget both auto-merged onto the same grid cell x:12, y:122 (full overlap → fails the non-overlap drift test).	Moved this PR's widget to the next free row x:0, y:130 (per the PR body's own conflict-note guidance); left main's widget where main put it.
3	Semantic (coverage)	The rename relocated this PR's new content_class = articleHost === null ? null : … ternary into a package that enforces 100% branch coverage independently, orphaning the null-branch's covering test (a hutch integration test that stayed behind). Branch coverage dropped to 98.24%.	Restored the PR's original intent with an equivalent unit test in analytics.test.ts (malformed URL → article_host/content_class = null). Back to 100%.

The PR's substantive changes (articleHostFromSubmitted, the nullable event fields, all 7 emitSaveIntent(error) calls, the generate-summary DLQ fix) followed the rename into @packages/web-analytics intact — no code reverted.

Verification — pnpm nx affected -t check (env loaded from .envrc): all 4 affected projects green (hutch, save-link, @packages/web-analytics, blog-site), all coverage thresholds met (100% on web-analytics). A dry-run merge against the latest origin/main (3c527309) is conflict-free.

Note: two intermittent failures surfaced during verification, both unrelated to this PR and resolved as environment issues — a stale node_modules missing the @packages/domain → @packages/escape-regexp workspace symlink (fixed with pnpm install --frozen-lockfile; a dependency edge main added that the cached checkout predated), and hutch route tests needing STATIC_BASE_URL from .envrc. nx flagged both as flaky; both passed on retry.
· fix/save-pipeline-visibility

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop #937
• Commit: c29b75e

[claude]

Claude finished @FagnerMartinsBrack's task in 4m 34s —— View job

---

PR Review: fix(save-pipeline): count user-facing save failures + stop the generate-summary DLQ loop

• Read reviewer instructions + CLAUDE.md guidelines
• Gather PR diff (commit c29b75e2, post-rebase) and commit history
• Review save-intent instrumentation (web-analytics package, queue route)
• Review generate-summary DLQ-loop fix
• Review dashboard widget + drift tests
• Post final review

Re-review of the rebased/conflict-resolved commit c29b75e2 (was 68bd979d). I focused on whether the rebase onto main (the @packages/web-analytics move + the blog-traffic widget from #941) preserved the PR's intent and didn't introduce regressions, then re-verified the two load-bearing claims.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. The two outcome-agnostic save-intent widgets fold validation errors into saves (unchanged from prior reviews)

analytics-dashboard.ts:428 ("Save-intent by surface × content class") and :443 ("…by content class %") filter only on event = view_save_intent with no outcome filter. This PR turns outcome=error from ~0 into a common event, so both widgets' saves totals now include validation errors, and the pie gains a first-time unclassified slice (error events carry content_class = null). Counting all intents is these widgets' established design (they already fold saved + prompted_to_sign_up), so this is plausibly intended — but the saves alias becomes a slight misnomer and the own-vs-third-party ratio is diluted by unclassifiable malformed-URL errors. If they should track attempted/successful saves, add and outcome <> "${SAVE_OUTCOMES.error}" to both. Fix this →

2. Drift annotation still counts the new widget as save-funnel, but the code places it under Homepage A/B (review #2 finding — the rebase moved the y-coord but not the block)

The conflict resolution moved the "Save errors by surface" widget from y:122 → y:130 to clear the overlap with #941's blog-pageviews widget, but left it physically inside the Homepage A/B widgets.push(...) block (analytics-dashboard.ts:589-602), under the // --- Homepage A/B experiment --- comment (:568). The drift annotation now reads …4 save-funnel …, 1 homepage-ab, 1 blog-traffic (analytics-dashboard.test.ts:72), but physically the save-funnel push (:425-470) holds 3 widgets and the Homepage A/B push holds 2. The total (29) is correct and the drift test still guards it — only the human-readable section mapping is off, so a future reader finds a save-errors widget under an A/B-experiment comment. Cleanest fix: move the logWidget object into the save-funnel widgets.push block near :469, keeping x:0, y:130. Fix this →

3. No dedicated test for the preserved crawl=pending race (unchanged from prior reviews)

The new handler branch keeps the assert→DLQ path for the genuine race (crawl=pending, content absent). The PR adds crawl=failed→skipped, crawl=unsupported→skipped, and the no-row DLQ case — good — but crawl=pending is still only covered transitively by the existing "content not found" fixture, which is crawl: { kind: "ready" } (generate-summary-handler.test.ts:43) — i.e. it exercises the ready inconsistency, not the pending race. A dedicated crawl=pending case would document the exact branch the PR calls out. Minor. Fix this →

4. PR-body "out of scope" rationale for recrawlTieKeptCanonical is inaccurate (doc-only, no code impact; unchanged from prior reviews)

The body says markCrawlExhausted's summary=failed(crawl-failed) write is left untouched "converting it to skipped would disable recrawlTieKeptCanonical's recovery predicate". But this PR's new branch flips those same rows to summary=skipped(crawl-failed) on the first auto-heal round anyway, so the predicate (summary.kind === "failed") stops matching them regardless. No regression — the branch runs only when !article, and tie-kept recovery fires only when canonical already exists (a later-recovered row re-primes via recrawlPromoteTier) — but the stated mitigation doesn't actually preserve that path. Worth correcting the note so a future reader doesn't rely on the failed state for recovery.

---

Verification notes (independently re-checked on c29b75e2)

Rebase integrity — the conflict resolution is clean:

• The substantive changes survived the move into @packages/web-analytics intact: articleHostFromSubmitted (null-safe host parse), the article_host/content_class widening to nullable, and all 9 emitSaveIntent(...error) call sites in queue.page.ts are present and correct.
• Coverage of the new null-branch is restored. The rename relocated content_class: articleHost === null ? null : classifyContentSource(...) into a package that enforces 100% branch coverage independently; the new unit test analytics.test.ts:404 (buildIntent({ url: "not a url" }) → article_host/content_class = null) covers the null branch, and existing example.com cases cover the non-null branch. articleHostFromSubmitted has full branch coverage (valid / unparseable / empty string / empty host via file:///).
• Widget count + grid are consistent. Total is 29 (guarded by the drift test). No overlaps: Homepage A/B x[0–12] y[122–130], Blog pageviews x[12–24] y[122–130], Save-errors x[0–12] y[130–138].

DLQ loop terminates, no spurious side-effect. The new !article && existing && crawl ∈ {failed, unsupported} branch calls markSummarySkipped (terminal); decideSummaryAutoHeal returns skip for anything not summary=failed, and the handler's cache-hit check short-circuits skipped, so the reprime loop closes. The branch runs even when summary=failed (cache-hit only short-circuits ready/skipped), which is exactly the stuck population. crawl=pending (race) and crawl=ready (inconsistency) still take the assert→DLQ path. crawl-failed/crawl-unsupported are valid SummarySkipReasons.

Save-error emits are clean — no double-emit. In the save-content handler (the densest path): each of the five guard branches (:1005, :1017, :1030, :1048, :1061) emits error then res.status(422) + return; the success path (:1074 saved → 201) is reachable only when all guards pass, and the catch (:1078 error) is mutually exclusive with the success res. Same shape on save-article, save-html, and queue/save. New route tests correctly distinguish malformed-URL (article_host: null) from schema-valid-but-unsaveable (article_host: "localhost"/"example.com").

Deliberate omissions (multipart-parse 422, 406, bulk per-entry skips, async crawl failures) match the PR description. Solid, well-scoped, well-tested PR; the rebase was handled correctly. All four notes above are optional.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.

_{Note: this is a manual @claude review via the listener (not the automated reviewer workflow), so I've posted the full review here in the tracking comment — the only channel visible in this flow — rather than deferring to the auto-apply workflow.}
· fix/save-pipeline-visibility

[FagnerMartinsBrack]

⚠️ Auto-fix skipped: the review run produced no parseable review block (missing CLAUDE_REVIEW_START/END): no CLAUDE_REVIEW block found in the run output. The full review is in the run's tracking comment — address any high/medium findings manually.