feat(data-modeling): notify when a materialized view starts failing - #80689
Conversation
Send an in-app notification and an optional immediate email the first time a materialized view starts failing, instead of only the next daily digest. Repeats of an ongoing failure streak stay quiet. Users pick daily digest or immediate per-failure email in their notification settings.
|
😎 Merged successfully - details. |
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 67.63 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 285.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 238.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.5 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.20 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.20 MiB · 17 files | no change | ████░░░░░░ 38.4% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 724.2 KiB | dist/toolbar/toolbar-app-Q7U4O3T2.css |
| 552.5 KiB | dist/toolbar/chunk-chunk-WQNFCK6N.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-2ENJVQK5.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-IB5P7RVW.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.2 KiB | dist/toolbar/toolbar-app-X6ES6NIN.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-VKNKEJQR.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-Q7KTDC2C.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +5.7 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1411.34 MiB · 🔺 +5.7 KiB (+0.0%)
⚠️ Backend snapshots — 1 updated (1 modified, 0 added, 0 deleted)
Query snapshots: Backend query snapshots updated
Changes: 1 snapshots (1 modified, 0 added, 0 deleted)
What this means:
- Query snapshots have been automatically updated to match current output
- These changes reflect modifications to database queries or schema
Next steps:
- Review the query changes to ensure they're intentional
- If unexpected, investigate what caused the query to change
⚠️ Backend coverage — 91.0% of changed backend lines covered — 9 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ██████████████████░░ 91.0% (94 / 103)
| File | Patch | Uncovered changed lines |
|---|---|---|
posthog/tasks/email.py |
82.0% | 150, 157–159, 1041, 1045–1047, 1053 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 31502465065 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.5% |
8,852 / 22,431 |
warehouse_sources_queue |
█████████░░░░░░░░░░░ 45.9% |
90 / 196 |
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
data_tools |
█████████████░░░░░░░ 66.2% |
90 / 136 |
tasks |
██████████████░░░░░░ 70.2% |
36,203 / 51,546 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
█████████████████░░░ 82.6% |
27,324 / 33,083 |
apm |
█████████████████░░░ 83.8% |
1,220 / 1,455 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,649 |
data_modeling |
█████████████████░░░ 86.5% |
8,459 / 9,782 |
notebooks |
█████████████████░░░ 86.6% |
8,103 / 9,361 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
wizard |
█████████████████░░░ 86.7% |
1,090 / 1,257 |
cohorts |
██████████████████░░ 87.6% |
6,482 / 7,400 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
managed_warehouse |
██████████████████░░ 88.1% |
6,057 / 6,876 |
data_warehouse |
██████████████████░░ 88.6% |
11,731 / 13,246 |
business_knowledge |
██████████████████░░ 89.0% |
4,384 / 4,928 |
exports |
██████████████████░░ 89.2% |
7,528 / 8,444 |
engineering_analytics |
██████████████████░░ 89.3% |
6,759 / 7,570 |
dashboards |
██████████████████░░ 89.4% |
6,004 / 6,713 |
visual_review |
██████████████████░░ 89.5% |
5,870 / 6,558 |
alerts |
██████████████████░░ 90.3% |
4,482 / 4,966 |
mcp_analytics |
██████████████████░░ 90.4% |
3,968 / 4,388 |
conversations |
██████████████████░░ 90.5% |
18,390 / 20,315 |
links |
██████████████████░░ 90.6% |
183 / 202 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
canvas |
██████████████████░░ 91.1% |
2,234 / 2,452 |
error_tracking |
██████████████████░░ 91.1% |
11,208 / 12,298 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
slack_app |
██████████████████░░ 92.4% |
11,375 / 12,308 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
notifications |
███████████████████░ 92.7% |
1,022 / 1,103 |
early_access_features |
███████████████████░ 92.9% |
1,347 / 1,450 |
ai_observability |
███████████████████░ 92.9% |
17,197 / 18,507 |
mcp_store |
███████████████████░ 93.0% |
6,983 / 7,512 |
web_analytics |
███████████████████░ 93.1% |
15,971 / 17,162 |
surveys |
███████████████████░ 93.2% |
5,919 / 6,349 |
marketing_analytics |
███████████████████░ 93.2% |
15,752 / 16,893 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
approvals |
███████████████████░ 93.5% |
3,491 / 3,734 |
product_analytics |
███████████████████░ 93.6% |
7,114 / 7,600 |
legal_documents |
███████████████████░ 93.8% |
1,628 / 1,736 |
workflows |
███████████████████░ 94.3% |
7,969 / 8,454 |
endpoints |
███████████████████░ 94.3% |
8,814 / 9,349 |
tracing |
███████████████████░ 94.4% |
2,678 / 2,837 |
review_hog |
███████████████████░ 94.6% |
8,246 / 8,715 |
skills |
███████████████████░ 94.7% |
3,862 / 4,077 |
messaging |
███████████████████░ 95.4% |
3,401 / 3,565 |
replay_vision |
███████████████████░ 95.5% |
19,048 / 19,941 |
logs |
███████████████████░ 95.6% |
11,834 / 12,382 |
experiments |
███████████████████░ 95.8% |
28,704 / 29,965 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
data_quality |
███████████████████░ 96.4% |
747 / 775 |
feature_flags |
███████████████████░ 96.5% |
17,825 / 18,478 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 96.5% |
12,266 / 12,707 |
growth |
███████████████████░ 96.7% |
5,807 / 6,008 |
access_control |
███████████████████░ 96.9% |
870 / 898 |
warehouse_sources |
████████████████████ 97.5% |
388,280 / 398,221 |
data_catalog |
████████████████████ 97.9% |
2,677 / 2,734 |
analytics_platform |
████████████████████ 98.1% |
2,254 / 2,298 |
metrics |
████████████████████ 98.2% |
2,491 / 2,536 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
⚠️ Django migration SQL — 1 new migration to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/notifications/backend/migrations/0021_alter_notificationevent_notification_type.py
BEGIN;
--
-- Alter field notification_type on notificationevent
--
-- (no-op)
COMMIT;Last updated: 2026-08-11 14:40 UTC (1a51990)
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 0 Safe | 1 Needs Review | 0 Blocked
⚠️ Needs Review
May have performance impact
notifications.0021_alter_notificationevent_notification_type
└─ #1 ⚠️ AlterField
Field alteration may cause table locks or data loss (check if changing type or constraints)
model: notificationevent, field: notification_type, field_type: CharField
Last updated: 2026-08-11 14:41 UTC (1a51990)
Replace the daily-or-immediate picker with two independent switches, so someone can keep the digest and also hear about each view as it starts failing.
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 2 should fix, 1 consider. Published 3 findings (view the review). |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 3 issues
Files (9)
frontend/src/lib/components/NotificationsMenu/NotificationRow.tsxfrontend/src/lib/components/NotificationsMenu/notificationToasts.tsxfrontend/src/scenes/settings/user/UpdateEmailPreferences.tsxfrontend/src/types.tsposthog/models/user.pyposthog/tasks/email.pyposthog/temporal/data_modeling/activities/fail_materialization.pyproducts/notifications/backend/facade/enums.pyproducts/notifications/backend/migrations/0021_alter_notificationevent_notification_type.py
| previous_job = _get_previous_jobs(saved_query.id, job.id, 1).first() | ||
| if previous_job is not None and previous_job.status == DataModelingJobStatus.FAILED: | ||
| return False |
There was a problem hiding this comment.
Streak detection treats a cancelled (preempted) run as ending the failure streak, causing repeat notifications for an unresolved failure
Why we think it's a valid issue
- Checked:
_maybe_notify_materialization_failurestreak logic and_get_previous_jobsexclusion set (fail_materialization.py); wherecancelled=Trueoriginates (workflows/materialize_view.py:288-314); how CANCELLED is persisted (_fail_node_and_data_modeling_job, fail_materialization.py:91,106); and schedule overlap policies. - Found (premise confirmed):
_get_previous_jobsonly does.exclude(status=SKIPPED)— CANCELLED is not excluded._maybe_notify_materialization_failurereads just the single most-recent prior job and suppresses onlyif previous_job.status == FAILED. So a CANCELLED job sitting between two FAILED runs is returned asprevious_job, its status != FAILED, and the current FAILED job is treated as a fresh streak start → re-sends the in-app notification and immediate email for an unresolved failure. - Found (CANCELLED is real and interleaves):
materialize_view.pysetscancelled=Trueon TemporalCancelledErrorand callsfail_materialization_activity, which writes astatus=CANCELLEDjob row. The per-saved-query schedule usesScheduleOverlapPolicy.CANCEL_OTHER(products/data_warehouse/backend/logic/data_load/saved_query_service.py:68), so a new scheduled run cancels an in-flight one — exactly the FAILED→CANCELLED→FAILED interleaving the finding describes for a tight-cadence view. Worker-restart/deploys are an additional cancellation source. (Note: DAG/v2 schedules useScheduleOverlapPolicy.SKIP, and SKIPPED is already excluded, so overlap-preemption on that path is harmless — the CANCELLED gap remains via CANCEL_OTHER per-view schedules and deploy/worker cancellation.) - Found (consistent with author intent): The PR body states "Cancelled runs never notify — preemption is our doing," and the
_get_previous_jobscomment excludes SKIPPED because it "is evidence of neither health nor failure." The identical reasoning applies to CANCELLED, so this is a genuine inconsistency in the feature's own design, not speculative paranoia. - Impact: Real correctness bug directly undermining the PR's headline design goal ("stays quiet while the streak continues"): a duplicate notification + email for a still-broken query. Named trigger, named consequence — clears the keep bar. Impact is bounded to notification noise (no data/security/corruption), and I keep
should_fixsince a spurious "as it happens" email is precisely the noise this feature was built to prevent, the trigger is realistic, and the fix is cheap and well-scoped (skip CANCELLED alongside SKIPPED without altering the shared helper's timeout-path semantics).
Issue description
_maybe_notify_materialization_failure decides whether this is the "first failure of a streak" by looking at only the single most recent prior job (_get_previous_jobs(saved_query.id, job.id, 1)) and suppressing notification only if previous_job.status == DataModelingJobStatus.FAILED. _get_previous_jobs excludes SKIPPED jobs (so a skipped run can't reset a streak) but does NOT exclude CANCELLED jobs. CANCELLED is a real, non-hypothetical outcome here: MaterializeViewWorkflow marks a job cancelled=True whenever the Temporal activity is preempted (e.g. a new scheduled run supersedes an in-flight one on a saved query whose materialization takes longer than its own schedule interval — precisely the kind of tight-cadence view this feature is meant to protect).
Concretely: Job A fails on saved query Q (streak starts, notification + immediate email sent). The next scheduled run for Q is cancelled due to preemption (job B, CANCELLED, correctly produces no notification per the if not inputs.cancelled guard at the call site). The following run for Q fails again with the same underlying broken query (job C). _get_previous_jobs returns job B (the most recent non-skipped job) as the "previous job"; since B.status != FAILED, the code treats job C as the start of a brand-new streak and re-sends the in-app notification and the immediate email — even though the query never actually recovered. This directly undermines the PR's stated design goal ("stays quiet while the streak continues... notifying on the first failure of a streak, rather than every failed run") for a saved query whose failures happen to interleave with preemptions, which is a plausible pattern for a broken query on a tight schedule.
Suggested fix
When determining the previous job for streak purposes, skip over CANCELLED jobs the same way SKIPPED jobs are already skipped, since a cancellation is "our doing" and carries no information about whether the query itself recovered. Avoid changing the shared _get_previous_jobs helper's exclusion set directly (it's also used by should_pause_schedule_for_timeout, which has different, intentional semantics for encountering a non-FAILED status). Instead, either add an exclude_statuses parameter to _get_previous_jobs used only from _maybe_notify_materialization_failure (excluding both SKIPPED and CANCELLED), or loop through a small window of recent jobs in _maybe_notify_materialization_failure and use the first one that is not SKIPPED/CANCELLED as the actual "previous outcome" for the streak comparison.
Prompt to fix with AI (copy-paste)
## Context
@posthog/temporal/data_modeling/activities/fail_materialization.py#L168-170
<issue_description>
`_maybe_notify_materialization_failure` decides whether this is the "first failure of a streak" by looking at only the single most recent prior job (`_get_previous_jobs(saved_query.id, job.id, 1)`) and suppressing notification only `if previous_job.status == DataModelingJobStatus.FAILED`. `_get_previous_jobs` excludes `SKIPPED` jobs (so a skipped run can't reset a streak) but does NOT exclude `CANCELLED` jobs. `CANCELLED` is a real, non-hypothetical outcome here: `MaterializeViewWorkflow` marks a job `cancelled=True` whenever the Temporal activity is preempted (e.g. a new scheduled run supersedes an in-flight one on a saved query whose materialization takes longer than its own schedule interval — precisely the kind of tight-cadence view this feature is meant to protect).
Concretely: Job A fails on saved query Q (streak starts, notification + immediate email sent). The next scheduled run for Q is cancelled due to preemption (job B, `CANCELLED`, correctly produces no notification per the `if not inputs.cancelled` guard at the call site). The following run for Q fails again with the *same* underlying broken query (job C). `_get_previous_jobs` returns job B (the most recent non-skipped job) as the "previous job"; since `B.status != FAILED`, the code treats job C as the start of a brand-new streak and re-sends the in-app notification and the immediate email — even though the query never actually recovered. This directly undermines the PR's stated design goal ("stays quiet while the streak continues... notifying on the first failure of a streak, rather than every failed run") for a saved query whose failures happen to interleave with preemptions, which is a plausible pattern for a broken query on a tight schedule.
</issue_description>
<issue_validation>
- **Checked:** `_maybe_notify_materialization_failure` streak logic and `_get_previous_jobs` exclusion set (fail_materialization.py); where `cancelled=True` originates (workflows/materialize_view.py:288-314); how CANCELLED is persisted (`_fail_node_and_data_modeling_job`, fail_materialization.py:91,106); and schedule overlap policies.
- **Found (premise confirmed):** `_get_previous_jobs` only does `.exclude(status=SKIPPED)` — CANCELLED is not excluded. `_maybe_notify_materialization_failure` reads just the single most-recent prior job and suppresses only `if previous_job.status == FAILED`. So a CANCELLED job sitting between two FAILED runs is returned as `previous_job`, its status != FAILED, and the current FAILED job is treated as a fresh streak start → re-sends the in-app notification and immediate email for an unresolved failure.
- **Found (CANCELLED is real and interleaves):** `materialize_view.py` sets `cancelled=True` on Temporal `CancelledError` and calls `fail_materialization_activity`, which writes a `status=CANCELLED` job row. The per-saved-query schedule uses `ScheduleOverlapPolicy.CANCEL_OTHER` (products/data_warehouse/backend/logic/data_load/saved_query_service.py:68), so a new scheduled run cancels an in-flight one — exactly the FAILED→CANCELLED→FAILED interleaving the finding describes for a tight-cadence view. Worker-restart/deploys are an additional cancellation source. (Note: DAG/v2 schedules use `ScheduleOverlapPolicy.SKIP`, and SKIPPED is already excluded, so overlap-preemption on that path is harmless — the CANCELLED gap remains via CANCEL_OTHER per-view schedules and deploy/worker cancellation.)
- **Found (consistent with author intent):** The PR body states "Cancelled runs never notify — preemption is our doing," and the `_get_previous_jobs` comment excludes SKIPPED because it "is evidence of neither health nor failure." The identical reasoning applies to CANCELLED, so this is a genuine inconsistency in the feature's own design, not speculative paranoia.
- **Impact:** Real correctness bug directly undermining the PR's headline design goal ("stays quiet while the streak continues"): a duplicate notification + email for a still-broken query. Named trigger, named consequence — clears the keep bar. Impact is bounded to notification noise (no data/security/corruption), and I keep `should_fix` since a spurious "as it happens" email is precisely the noise this feature was built to prevent, the trigger is realistic, and the fix is cheap and well-scoped (skip CANCELLED alongside SKIPPED without altering the shared helper's timeout-path semantics).
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
When determining the previous job for streak purposes, skip over `CANCELLED` jobs the same way `SKIPPED` jobs are already skipped, since a cancellation is "our doing" and carries no information about whether the query itself recovered. Avoid changing the shared `_get_previous_jobs` helper's exclusion set directly (it's also used by `should_pause_schedule_for_timeout`, which has different, intentional semantics for encountering a non-`FAILED` status). Instead, either add an `exclude_statuses` parameter to `_get_previous_jobs` used only from `_maybe_notify_materialization_failure` (excluding both `SKIPPED` and `CANCELLED`), or loop through a small window of recent jobs in `_maybe_notify_materialization_failure` and use the first one that is not `SKIPPED`/`CANCELLED` as the actual "previous outcome" for the streak comparison.
</potential_solution>
There was a problem hiding this comment.
Fixed. The notify path calls _get_previous_jobs(..., ignore_cancelled=True), so a preemption no longer restarts the streak. The timeout counter still treats a cancellation as a break, which is the conservative reading there. Test added.
Both the email tasks and the in-app notification now drop members who cannot open the failing view, so a saved query name is not disclosed to someone denied on it. The email path gains a shared filter that checks the warehouse resource and then the specific query; the in-app path passes a resolver that applies the same per-object check. Also hoists the data-modeling imports in the email module and its tests to module scope.
A cancelled run no longer restarts the notification streak, so a preemption or a deploy in the middle of an ongoing failure stops producing a second notification for the same unresolved problem. The timeout counter keeps treating a cancellation as a break, which is the conservative reading there. The notification also moves out of the error-specific recovery block. A failing pause or revert is exactly when someone most needs telling, and it used to take the notification down with it.
| <SimpleSwitch | ||
| setting="materialized_view_sync_failed_immediate" | ||
| label="Right away" | ||
| description="An email as soon as a view starts failing. Later failures of the same view are not emailed." |
There was a problem hiding this comment.
Is the intention with not re-emailing when a view fails again in the future that the user should enable the daily digest to see that?
There was a problem hiding this comment.
Yes - the immediate email is the "it just broke" signal, so if something keeps failing every 15 minutes - they don't get 12908319208 emails for the same error. Once they fix the matview, and we see a status that's not a failure - that sort of resets it and they will get an immediate email again.
| for membership in get_members_to_notify(team, NotificationSetting.MATERIALIZED_VIEW_SYNC_FAILED.value) | ||
| if membership.user.notification_settings.get("materialized_view_sync_failed_daily", True) |
There was a problem hiding this comment.
This logic is a little strange:
- This notification is only sent if the user has 2 different notification settings enabled
- One check goes to the correct path of calling
get_members_to_notify, and the other re-implements the same check in the if statement.
I think a more stable approach here would be to call get_members_to_notify twice, and then take the intersection of the results if the intention is to require both notification settings to be enabled. If the intention is that you only need the materialized_view_sync_failed_daily setting enabled, then just call get_members_to_notify(team, NotificationSetting.MATERIALIZED_VIEW_SYNC_FAILED_DAILY)
There was a problem hiding this comment.
Both settings are required, so it now calls get_members_to_notify twice and intersects, in get_members_to_notify_of_matview_failure.
Add the two delivery settings to the NotificationSetting enum, resolve recipients through get_members_to_notify for both of them, drop an unneeded cast, and remove an accidentally committed file.
The two delivery settings joined NOTIFICATION_DEFAULTS, so the user API's expected settings dicts had to list them too.
A job left Running by a dead worker read as "not Failed", so the next failure looked like the start of a new streak and sent a second email for a view that never recovered. The notification lookup now skips Running alongside Cancelled, since neither says whether the query recovered.
|
/trunk merge |
2 participants
Problem
A materialized view that stops refreshing is silent. Someone whose model is on a 15-minute cadence finds out from a once-a-day digest email, or from stale numbers in a dashboard.
The digest stays the right default for most people. What is missing is a faster signal for anyone who wants one, and any signal at all inside the app.
Changes
Notifying on the first failure of a streak, rather than every failed run, is the design decision worth a reviewer's attention. A view failing on a 15-minute cadence produces 96 failures a day; all of them describe the same broken query. The activity compares against the previous job for that saved query and stays quiet while the streak continues. A run that succeeds and later fails again is a new streak, and notifies again.
A cancelled or abandoned run stays silent, and does not re-arm the next notification. Neither status says whether the query recovered.
The frontend change is two switches in user notification settings, shown only when the materialization-failure setting is already on.
The in-app notification, seeded locally against a demo project:
How did you test this code?
Ran the local stack and drove both surfaces in a browser.
Notification settings: walked all four combinations of the two switches and confirmed each one persists by re-reading the stored settings. Screenshots above.
In-app notification: seeded one
materialization_failurenotification and opened the notifications panel. It renders with the view name in the title and the run error in the body. Screenshot above.posthog/tasks/test/test_email.py— parameterized cases pin each switch against the digest task and the per-failure task, including both-on and each-alone, and that the master switch off wins over either. The digest previously had no notion of per-user delivery choice.posthog/temporal/tests/data_modeling/test_materialize_view_activities.py— parameterized over the previous job's status (none / completed / failed) to pin the streak rule, plus parameterized cases that a cancelled or abandoned run stays silent. These catch the regression that matters here: a failing view spamming a notification every run.uv run mypy --cache-fine-grained .clean repo-wide.pnpm --filter=@posthog/frontend typescript:checkclean repo-wide.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Skills invoked:
/writing-pr-descriptions.The work started from customer feedback asking to hear about materialization failures sooner than the daily digest. Nothing from that conversation is in this diff — no fixture, comment, or string is derived from it, and the tests use the repo's existing generic factories.
Two things changed across the session. An earlier shape notified on every failed run, which is unusable at sub-hourly cadence, so it became first-of-streak with the previous-job comparison. An earlier settings shape made digest and per-failure mutually exclusive; they are now two independent switches, because wanting a daily sweep and an immediate ping are not competing preferences.
One behavior worth knowing if a key is ever removed from the settings TypedDict: the API validates settings keys against a strict allowlist, and the frontend echoes the whole stored blob back on save, so a stored key that leaves the type makes every later save fail for users who already have it.
Open question for a reviewer: the setting is per user, not per view. Someone with one urgent model and fifty tolerant ones has to pick one mode for all of them.