Skip to content

fix(alerts): spread alert checks over a window after each interval - #106985

Draft
aspicer wants to merge 4 commits into
aspicer/jitter-basefrom
aspicer/jitter-alerts
Draft

aspicer wants to merge 4 commits into
aspicer/jitter-basefrom
aspicer/jitter-alerts

Conversation

@aspicer

@aspicer aspicer commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Alert checks bunch up at the top of the hour. An hourly alert keeps the minute it was created or last snapped to, and a quiet-hours window releases every alert behind it at the window's end, which is usually on the hour. Those checks reach ClickHouse in the same minutes as user queries. Split from #106458; merge after the helper in #106984.

Changes

Each automatic alert now checks at a stable offset after its interval boundary, derived from the alert id. Hourly alerts check between :02 and :13, and 15-minute alerts one to three minutes after each quarter hour. Daily, weekly and monthly alerts check at minutes 2 to 59 of their existing anchor hour. After a quiet window, a check keeps its offset from the window's end unless that lands in another quiet window.

An explicit start time keeps the time the user picked, and real-time alerts are unchanged. Existing alerts pick up their offset at their next check or skip, so no migration is needed. The alert form shows the automatic window instead of one approximate time, with a line saying each alert keeps a consistent minute.

Note

Hourly alert notifications can arrive up to 13 minutes after the hour, instead of at the minute they arrive today.

The six-hourly query upgrade schedule, which product analytics also owns, starts two minutes after its boundary with up to 30 minutes of jitter.

Alert preview: before, after, and narrow

Before
After
Narrow

How did you test this code?

The scheduling and quiet-hours unit tests pass locally. A DST case covers Lord Howe's 30-minute change and New York's repeated hour. The alert form Jest test passed locally before review; its new rows for explicit daily, weekly and monthly start times run on CI only, because this worktree has no frontend dependencies. The tests pin the window bounds, one stable minute per alert, skipped backlogs, custom start times and quiet-hours snaps. The API, activity and 15-minute suites need Postgres, which was down locally; CI runs them, and they passed with the same code on #106458.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

Companion public docs: PostHog/posthog.com#20465, to publish after this deploys.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5; Codex, GPT-6 (review fixes on #106458).

Split from #106458 by owning team: the owners.yaml resolver decided ownership, and a team gets its own PR when its owned files clear the reviewer assigner's bar of 10 lines or 3 files. The code is unchanged from the reviewed head of #106458, except that its signals test-fixture fix dropped because #106460 fixed the same bug on master. Claude Opus 5.5 (claude-opus-5-5) then added two review fixes, using writing-tests, writing-code-comments and writing-pr-descriptions. Hourly and 15-minute interval starts now come from the local wall clock, and the preview shows an explicit start time for calendar alerts. CodeRabbit CLI ran once with --deep over all the split PRs combined on master 8475a79 and reported no findings. Skills for the split: stacking-prs, establishing-code-ownership, reviewing-with-coderabbit, writing-pr-descriptions. The original change also used qa-team, announcing-behavior-changes, writing-tests, writing-code-comments, writing-ui-components, writing-user-facing-copy, running-ci-preflight and debugging-ci-failures.

Each alert now checks at a stable offset after its interval boundary,
derived from the alert id: 2 to 13 minutes past the hour for hourly
alerts, 1 to 3 minutes into each quarter for 15-minute alerts, and
minutes 2 to 59 of the anchor hour for daily, weekly and monthly alerts.
An explicit schedule_start_time keeps the time the user chose. Real-time
alerts are unchanged. Existing alerts pick up their offset at their next
check, so no migration is needed. The form previews the check window.

The six-hourly query upgrade schedule also moves off minute zero.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aspicer aspicer self-assigned this Sep 25, 2026
@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

⚠️ Complexity (TypeScript) — 2 functions above the limit (max 20)

Cyclomatic complexity above the limit in changed typescript files (10 for production files, 15 for test files). Warn only: worth simplifying when you next touch these functions.

Function Location Complexity Limit
AlertIntervalRow products/alerts/frontend/components/AlertIntervalRow.tsx:60 20 10
approximateNextAlertRun products/alerts/frontend/logic/alertSchedulingStale.ts:33 18 10
✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

🚨 Comment density — 6% of added code lines are comments (26 of 416)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/alerts/backend/test/test_scheduling.py 9 111
products/alerts/frontend/logic/alertSchedulingStale.ts 6 76
products/alerts/backend/facade/scheduling.py 5 77
posthog/tasks/alerts/schedule_restriction.py 2 9
posthog/tasks/alerts/test/test_schedule_restriction.py 2 26
products/alerts/frontend/logic/alertSchedulingStale.test.ts 2 74

This check does not block merging. It updates on every push and clears when the share drops.

⚠️ Bundle size — 🔺 +1.3 KiB (+0.0%)

Uncompressed size of every built .js bundle, compared against the base branch.

Total: 68.86 MiB · 🔺 +1.3 KiB (+0.0%)

File Size Δ vs base
exporter/_parent/products/alerts/frontend/views/EditAlertModal.js 109.7 KiB 🔺 +1.3 KiB (+1.2%)

Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report

✅ Eager graph — within budget

How 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 import() / React.lazy chunks are not counted.

Root Eager (shipped) Δ vs base Budget
entry (logged-out pages, app bootstrap)
src/index.tsx
1.57 MiB · 22 files no change █████████░ 85.2% of 1.84 MiB
logged-out boot: index + App + bootApp (preloaded by every page, including /login)
src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
3.57 MiB · 628 files no change █████████░ 88.7% of 4.03 MiB
authenticated shell (every logged-in page)
src/scenes/AuthenticatedShell.tsx
7.27 MiB · 2,301 files no change █████████░ 87.2% of 8.34 MiB

🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx

Largest files eagerly shipped from src/index.tsx
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/index.tsx
1.3 KiB src/RootErrorBoundary.tsx
912 B ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js
854 B src/scenes/ChunkLoadErrorBoundary.tsx
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
Size File
301.8 KiB ../node_modules/.pnpm/posthog-js@1.434.13_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
267.6 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
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
100.4 KiB src/lib/api.ts
84.9 KiB src/products.tsx
69.1 KiB src/lib/lemon-ui/icons/icons.tsx
63.9 KiB src/lib/utils/eventUsageLogic.ts
38.7 KiB ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js
33.9 KiB ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js
28.3 KiB src/scenes/scenes.ts
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
Size File
301.8 KiB ../node_modules/.pnpm/posthog-js@1.434.13_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
271.7 KiB src/taxonomy/core-filter-definitions-by-group.json
267.6 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
153.7 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
100.4 KiB src/lib/api.ts
98.5 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
84.9 KiB src/products.tsx

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.37 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.37 MiB · 19 files no change ████░░░░░░ 41.4% of 5.72 MiB
Deferred (lazy) 2.10 MiB · 44 files no change n/a — loads on demand
Loader dist/toolbar.js 1.2 KiB no change █░░░░░░░░░ 6.0% of 19.5 KiB
Largest eagerly-shipped chunks
Size File
791.6 KiB dist/toolbar/toolbar-app-EBOBUBAP.css
650.5 KiB dist/toolbar/chunk-chunk-UTBICQWM.js
483.6 KiB dist/toolbar/chunk-chunk-LP5DDLVQ.js
138.3 KiB dist/toolbar/chunk-chunk-PFYG7BOO.js
131.8 KiB dist/toolbar/chunk-chunk-FDH2IBXT.js
75.2 KiB dist/toolbar/toolbar-app-TC5H7QLB.js
69.0 KiB dist/toolbar/chunk-chunk-TSAL54PB.js
35.6 KiB dist/toolbar/chunk-chunk-ARCGBV2S.js
21.0 KiB dist/toolbar/chunk-chunk-JOFF6CFX.js
6.8 KiB dist/toolbar/chunk-chunk-DV7IWQNF.js

Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile

✅ Dist folder size — 🔺 +20.1 KiB (+0.0%)

Total size of the built frontend/dist folder (all assets), compared against the base branch.

Total: 944.11 MiB · 🔺 +20.1 KiB (+0.0%)

@greptile-apps

greptile-apps Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Changes alert scheduling logic to spread checks over a time window.

The PR is not ready to merge until hourly alert scheduling preserves its cadence across partial-hour DST transitions.

Reviews (1) · Last reviewed commit: "fix(alerts): spread alert checks over a ..."

Comment thread products/alerts/backend/facade/scheduling.py Outdated
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

Alert checks now use deterministic per-alert offsets for automatic schedules. Schedule-restriction handling applies offsets when the adjusted time remains outside blocked windows. The frontend displays approximate execution-time ranges when runs can occur at different times. The upgrade-query schedule adds a two-minute offset and up to 30 minutes of jitter to its six-hour interval.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to dabdd

During a repeated daylight-saving hour, the planned evaluation shown for an automatic alert can be in the past. Actual alert scheduling is unaffected; this is a bounded display issue to fix or explicitly accept before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to dabdd

Alert checks will move to new execution windows, and a periodic maintenance job will gain jitter. The reviewed paths did not establish a new security exposure, but the breadth of the timing change and its deployment behavior warrant review.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The scheduling rule can affect automatic alerts across teams using this worker path, while each offset is derived from its alert ID. The evidence does not establish a new cross-tenant data or authority path.

Trust Boundaries and Controls

  • observed — The API associates a new alert with the request's team and user; worker preparation checks entitlement and validates the query before evaluation. The reviewed timing changes do not replace these controls.

Resilience and Maintainability Implications

  • observed — Completed checks advance next_check_at with alert state, and notification retries use targets_notified as a delivery sentinel. These controls limit repeated work but do not imply absolute check-row idempotency.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description is complete and standalone. It explains the problem, user-visible changes, testing scope and limitations, release status, documentation update, screenshots, and agent context. It also …
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/alerts/frontend/logic/alertSchedulingStale.ts-76-76 (1)

76-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Honor explicit start times for calendar alerts.

When schedule_start_time is set for a daily, weekly, or monthly alert, the backend schedules the next check at that time. The edit-form preview ignores it and displays the automatic calendar-anchor window instead. Handle explicit start times before applying the automatic window so the preview matches the scheduled time.

This is a preview correctness issue. It does not change when the backend runs the alert.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 91143a0d-264a-48b6-91ef-42f26d71fa68

📥 Commits

Reviewing files that changed from the base of the PR and between e107332 and 41ef018.

📒 Files selected for processing (12)
  • posthog/api/test/test_alert_15_minute_interval.py
  • posthog/tasks/alerts/schedule_restriction.py
  • posthog/tasks/alerts/test/test_schedule_restriction.py
  • posthog/tasks/alerts/utils.py
  • posthog/temporal/alerts/activities.py
  • posthog/temporal/schedule.py
  • products/alerts/backend/facade/scheduling.py
  • products/alerts/backend/test/test_scheduling.py
  • products/alerts/backend/tests/api/test_alert.py
  • products/alerts/frontend/components/AlertIntervalRow.tsx
  • products/alerts/frontend/logic/alertSchedulingStale.test.ts
  • products/alerts/frontend/logic/alertSchedulingStale.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.

@trunk-io

trunk-io Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

Failed Test Failure Summary Logs
Scenes-App/SidePanels SidePanelNotebooks smoke-test The test timed out while waiting for a loading indicator or spinner to disappear. Logs ↗︎

View Full Report ↗︎ ⋅ Docs

aspicer and others added 2 commits September 27, 2026 23:13
Hourly and 15-minute checks found the interval start by subtracting the
local minutes from the absolute time, and caught up by adding whole
intervals. With Lord Howe's 30-minute DST change that ran a second check
in the same local hour and skipped the next one. The start now comes
from the local wall clock, and the next check is the first local
interval start after the previous check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A daily, weekly, or monthly alert with a schedule start time runs at
exactly that local time, but the edit form preview showed the automatic
window instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 26fdd92d-baab-49bd-a0ce-a74a9b1e3fc4

📥 Commits

Reviewing files that changed from the base of the PR and between 41ef018 and 4803cf8.

📒 Files selected for processing (4)
  • products/alerts/backend/facade/scheduling.py
  • products/alerts/backend/test/test_scheduling.py
  • products/alerts/frontend/logic/alertSchedulingStale.test.ts
  • products/alerts/frontend/logic/alertSchedulingStale.ts

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.

Comment thread products/alerts/frontend/logic/alertSchedulingStale.ts
…ence

When a DST change repeats an alert's start time, the backend runs at the
first occurrence. dayjs picked either occurrence depending on the
browser's timezone, and its isAfter re-reads the wall time, so the
preview could show a run that had already passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟡 Minor · Avoid reparsing the current wall minute for automatic windows. · alertSchedulingStale.ts:70

products/alerts/frontend/logic/alertSchedulingStale.ts:70
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Avoid reparsing the current wall minute for automatic windows.

During the repeated hour, Day.js startOf('minute') can select the earlier occurrence. At 06:40 UTC, the hourly branch can then calculate 06:02–06:13 UTC (01:02–01:13 EST), which is already past. Floor the actual instant to the current minute before adding the next cadence offset.

Suggested fix
-        const anchor = localNow.startOf('minute').add(cadence - (localNow.minute() % cadence), 'minutes')
+        const currentMinute = dayjs(localNow.valueOf() - localNow.second() * 1000 - localNow.millisecond())
+        const anchor = currentMinute.add(cadence - (localNow.minute() % cadence), 'minutes')

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 23a568e9-ebd1-49a8-9996-0c0e417829b9

📥 Commits

Reviewing files that changed from the base of the PR and between 4803cf8 and dabdd68.

📒 Files selected for processing (2)
  • products/alerts/frontend/logic/alertSchedulingStale.test.ts
  • products/alerts/frontend/logic/alertSchedulingStale.ts

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 1 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant