feat(labeler): prolonged-error publisher notice via reconciliation cron (W10.5 follow-up)#2088
Conversation
…on (W10.5 follow-up) The sixth notification event. An assessment stuck in the terminal error state that stays the live, unsuperseded run escalates in two stages from the reconciliation cron: at 24h a high-severity assessment-prolonged-error operational_event so operators can triage an infra-vs-publisher cause, at 72h a neutral, actionable publisher notice through the W10.5 double-opt-in pipeline. - migration 0010: assessment_error_escalations tracking table makes each stage fire-once across the 5-minute cron ticks. - the notice reuses source_type 'issuance' keyed on the errored assessment id (an errored run never produces a block/warn notice, so no key collision and no notifications CHECK rebuild); resolveNoticeForSource routes error-state to the prolonged-error content for sweep parity. The finalization path stays silent on error (assessmentNoticeContent still returns null), so only the cron's 72h stage notifies. - supersession halts the ladder when a newer run exists for the same (uri,cid); deleted subjects are skipped; the scan drops fully-escalated rows so a mass-error backlog never starves newer errors. - the operator-alert insert is gated on the escalation still being un-alerted so overlapping cron ticks cannot double-alert; the publisher stage is idempotent via the notifications source claim. - notice carries no private detail; the operational_event payload is the public assessment id + cid only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Scope checkThis PR changes 866 lines across 8 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 8d70467 | Jul 17 2026, 06:53 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 8d70467 | Jul 17 2026, 06:53 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 8d70467 | Jul 17 2026, 06:53 AM |
There was a problem hiding this comment.
This PR adds a coherent two-stage escalation ladder for terminal error assessments: a 24h operator alert and a 72h publisher notice, driven from the reconciliation cron and made idempotent via a new tracking table. The approach fits the labeler’s existing patterns — it reuses the issuance notification source, the operational-events subsystem, and the double-opt-in send path, and it correctly keeps finalization silent for error runs.
I reviewed the diff statically (no test/lint runs) and focused on the new migration, cron pass, notification trigger changes, and call-sites. The code is generally careful: the operator-alert insert is gated on the unalerted escalation row and batched with the mark update, supersession/deleted-subject filtering is correct, and dropping fully-escalated rows prevents a backlog from starving newer errors.
I found two concrete issues that should be addressed before merge:
-
Missing index for the cron scan —
findEscalatableErrorsfiltersassessments.state = 'error' AND completed_at_epoch_ms <= ?and orders bycompleted_at_epoch_ms. The existingidx_assessments_state_createdonly covers(state, created_at_epoch_ms), so as error backlogs grow each 5-minute tick will scan/sort all error rows. This violates the repo’s index discipline and is a predictable operational regression. -
Publisher mark can stamp on transient failure —
notifyProlongedErroris wrapped inrunTrigger, which swallows all errors. The cron then unconditionally callsmarkPublisherNotified. If an aggregator read throws or a D1 write fails before anotificationsrow is claimed, no notice was sent and no row exists, but the error is permanently excluded from future passes. The self-healing contract only covers crashes after a row is claimed, not failures before a row exists.
Both are fixable in small, localized changes. No AGENTS.md-level concerns around SQL injection, authorization, localization, or changesets surfaced (the labeler app is private, and the emails are server-side English-only).
| publisher_notified_at_epoch_ms INTEGER, | ||
| created_at TEXT NOT NULL, | ||
| created_at_epoch_ms INTEGER NOT NULL | ||
| ); |
There was a problem hiding this comment.
[needs fixing] The findEscalatableErrors query filters assessments.state = 'error' AND assessments.completed_at_epoch_ms <= ? and orders by completed_at_epoch_ms ASC. The only existing assessment index is idx_assessments_state_created on (state, created_at_epoch_ms DESC), so SQLite can equality-match state but must still scan all errored rows and sort by completed_at. As the error backlog grows, a 5-minute cron tick becomes increasingly expensive.
That violates the repo’s index discipline (see 0006_assessment_checksum_index.sql for the same rationale). Add a partial index on completed_at_epoch_ms scoped to error rows:
| ); | |
| ); | |
| -- Supports the cron's state+completed-at scan in escalation order without a | |
| -- full table sort. Partial because only `error` rows are candidates. | |
| CREATE INDEX idx_assessments_error_completed | |
| ON assessments(completed_at_epoch_ms) | |
| WHERE state = 'error'; |
| await notifyProlongedError(deps, assessment); | ||
| await markPublisherNotified(deps.db, error.id, now); |
There was a problem hiding this comment.
[needs fixing] notifyProlongedError internally uses runTrigger, which catches and logs every error. The cron then unconditionally stamps publisher_notified_at. If the failure happened before sendNotification claimed a notifications row — for example an aggregator read threw, or a D1 write failed transiently — no source row exists, but the mark is still set. The next cron tick will skip this assessment forever, so a transient outage can permanently suppress the 72h publisher notice.
Stamp publisher_notified_at only when there is evidence the send actually reached the claim stage:
| await notifyProlongedError(deps, assessment); | |
| await markPublisherNotified(deps.db, error.id, now); | |
| await notifyProlongedError(deps, assessment); | |
| const claimed = await deps.db | |
| .prepare( | |
| `SELECT 1 FROM notifications WHERE source_type = 'issuance' AND source_id = ? LIMIT 1`, | |
| ) | |
| .bind(assessment.id) | |
| .first(); | |
| if (claimed) await markPublisherNotified(deps.db, error.id, now); |
This still lets the intended dedup/self-heal for undeliverable/pending/failed rows work, but it retries when the send never reached the database at all.
…send failure Addresses the emdashbot review on #2088. - Add a partial index idx_assessments_error_completed on assessments(completed_at_epoch_ms) WHERE state='error' so the 5-minute cron scan is index-backed instead of full-scanning error rows. - runTrigger now reports whether the trigger reached a terminal outcome (sent / confirmation / undeliverable / dedup hit) versus threw a transient error before any notifications row was claimed. The prolonged-error cron stamps publisher_notified_at only on a terminal outcome, so a transient pre-claim failure (aggregator read or D1 write throwing) leaves the row in the scan window to retry next tick rather than being permanently silenced. An unparseable subject URI counts as terminal (never retried). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both addressed (commit 8d70467). 1. Cron-scan index — added a partial index 2. Mark-on-transient-failure — good catch; this was a real hole. 13/13 prolonged-error + 161 across the shared- ~ 🤖 Claude Fable 5 |
There was a problem hiding this comment.
Re-review of PR #2088: both prior emdashbot[bot] findings are resolved.
-
Cron-scan index — migration
0010now adds the partial indexidx_assessments_error_completed ON assessments(completed_at_epoch_ms) WHERE state = 'error', which matches thefindEscalatableErrorsfilter andORDER BY a.completed_at_epoch_ms ASCexactly. This addresses the index-discipline concern from the previous review. -
Publisher mark on transient failure —
runTriggernow returns a boolean terminal/transient signal,notifyProlongedErrorpropagates it, andrunProlongedErrorEscalationonly callsmarkPublisherNotifiedwhen the trigger reached a terminal outcome. A pre-claim aggregator or D1 failure no longer permanently drops a publisher notice.
Statically, the rest of the ladder looks sound: the operator-alert insert is batched with the operator_alerted_at mark and guarded by gateOnUnalertedEscalation; supersession, deleted-subject filtering, and dropping fully-escalated rows are correct; the notice copy is public-safe; and the assessment-prolonged-error event carries only public identifiers. No AGENTS.md-level concerns around SQL interpolation, authorization, admin UI localization, changesets (labeler is private), or content-table locale filtering surfaced.
I did not find any remaining blocking issues. LGTM.
c065482
into
feat/plugin-registry-labelling-service
What does this PR do?
Implements the prolonged-error publisher notification — the sixth spec notification event, deferred during W10.5 because there was no stuck-run detection. Targets the
feat/plugin-registry-labelling-serviceintegration branch.An assessment can end in the terminal
errorstate (the labeler failed to complete its assessment — transient retries exhausted — not a finding against the publisher). Until now nothing told the publisher. This adds a two-stage escalation ladder driven by the reconciliation cron, for an errored run that stays the live, unsuperseded assessment for its subject:assessment-prolonged-erroroperational event, so operators can triage an infra-vs-publisher cause before anyone is emailed.The two thresholds and the operator-first ordering were ratified with the maintainer.
How it works:
0010_assessment_error_escalations.sql— a small tracking table making each stage fire-once across the 5-minute cron ticks.source_type = 'issuance'keyed on the errored assessment id. An errored run never produces a block/warn notice, so the key never collides with a finalization notice, and nonotifications.source_typeCHECK rebuild is needed.resolveNoticeForSourceroutes anerror-state assessment to the prolonged-error content, so the retry sweep re-renders identical copy.assessmentNoticeContentstill returns null forerror, sonotifyAssessmentOutcomeno-ops; only the cron's 72h stage notifies.(uri, cid); deleted subjects are skipped; the scan drops fully-escalated rows so a mass-error backlog never starves newer errors.Note: actual email is gated on the operator-owned Cloudflare Email Sending domain onboarding (a separate pre-launch item), same as the rest of W10.5. Migration
0010follows0009(the reconsideration table merged in #2083).An adversarial review pass found and fixed two MED issues before this PR: the escalation scan now drops fully-escalated rows (so a >200-error backlog can't starve newer errors), and the operator-alert insert is gated against overlapping cron ticks. A third finding (the operator head-start collapsing on rollout/outage-recovery of pre-existing >72h errors) was reviewed and consciously accepted by the maintainer; steady-state keeps the full 24h→72h head-start.
Part of #1909. Follows #2083.
Type of change
Checklist
pnpm typecheckpasses (all three labeler projects)pnpm lintpasses (0 diagnostics on touched files)pnpm testpasses (12 prolonged-error integration tests; regression across notification-triggers/sweep, reconciliation, operational-events, and the reconsideration suite green)pnpm formathas been run@emdash-cms/labelerisprivate: true.AI-generated code disclosure
Screenshots / test output
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
feat/labeler-prolonged-error. Updated automatically when the playground redeploys.