-
Notifications
You must be signed in to change notification settings - Fork 3.1k
chore(workflows): drop the dead email reputation snapshot table #75277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
meikelmosby
wants to merge
1
commit into
master
Choose a base branch
from
posthog-code/drop-emailreputationsnapshot-table
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+21
−1
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
20 changes: 20 additions & 0 deletions
20
products/workflows/backend/migrations/0017_drop_emailreputationsnapshot_table.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| from django.db import migrations | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("workflows", "0016_drop_emailreputationsnapshot_fk"), | ||
| ] | ||
|
|
||
| # Phase 2 of the two-phase table drop (safe-django-migrations.md, "Dropping Tables"). | ||
| # 0015 removed EmailReputationSnapshot from Django state and 0016 dropped its FK to | ||
| # posthog_hogflow; the physical table has been dead ever since. This drops it now that the | ||
| # state removal has shipped. Irreversible by nature — the reverse is a no-op rather than a | ||
| # bogus CREATE TABLE, so a rollback past this point simply leaves the table absent (nothing | ||
| # reads or writes it). | ||
| operations = [ | ||
| migrations.RunSQL( | ||
| sql='DROP TABLE IF EXISTS "posthog_emailreputationsnapshot";', | ||
| reverse_sql=migrations.RunSQL.noop, | ||
| ), | ||
| ] | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 0016_drop_emailreputationsnapshot_fk | ||
| 0017_drop_emailreputationsnapshot_table |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate Node.js/Temporal worker service actively reads and writes this table — not just the Python API endpoint
Why we think it's a valid issue
posthog-code/ses-tenant-event-comms, the stack tip), not local master. Verified the Nodeemail-reputation.service.ts, its Temporal worker, and the capability/mode/server wiring directly via the GitHub API on that ref.SELECT DISTINCT team_id FROM posthog_emailreputationsnapshot(email-reputation.service.ts:121), a secondSELECT DISTINCT ON (team_id) ... FROM posthog_emailreputationsnapshot(:418), andINSERT INTO posthog_emailreputationsnapshot ... ON CONFLICT DO NOTHING(:442). Nothing in the stack repointed or removed these.EmailReputationWorkerServiceis started inserver.ts:400-425undercapabilities.emailReputationEvaluator, the temporal worker registers a daily schedule (scheduleId: EMAIL_REPUTATION_SCHEDULE_ID,spec: { calendars: [{ hour }] }, email-reputation-worker.service.ts:118-135), and there is a dedicated deployment modePluginServerMode.email_reputation_evaluator = 'email-reputation-evaluator'(config.ts:54). The server.ts comment "Dedicated deployments should crash and restart" confirms a dedicated production deployment path (dev merely rides along viaisDevEnv()and degrades gracefully; dedicated deploymentsthrow).DROP TABLE IF EXISTS "posthog_emailreputationsnapshot"makes the next daily activity fail withrelation "posthog_emailreputationsnapshot" does not exist, producing repeated Temporal retries/failures — and at minimum breaks the local dev CDP pipeline where the capability defaults on. This is the genuine blind spot (invisible to Python-only investigation), the inverse of the first finding which was a false positive because the Python endpoint had actually been rewritten. The drop must wait until this Node evaluator is decommissioned or repointed.Issue description
Beyond the Python
HogFlowViewSet.team_reputationendpoint, there is an entirely separate, cross-runtime consumer ofposthog_emailreputationsnapshot: the Node.js Temporal workerEmailReputationService(nodejs/src/cdp/services/email-reputation/email-reputation.service.ts), registered as theemail-reputation-evaluatorcapability (nodejs/src/common/config.ts:54, nodejs/src/capabilities.ts:203, nodejs/src/server.ts:417, and nodejs/src/cdp/services/email-reputation/temporal/email-reputation-worker.service.ts:216) — a fully wired, deployed service, not dead code. Its own docstring says it 'runs as Temporal activities' with 'each daily run' appending snapshot rows, and its code does bothSELECT DISTINCT team_id FROM posthog_emailreputationsnapshot ...(line ~121) andINSERT INTO posthog_emailreputationsnapshot ... ON CONFLICT DO NOTHING(line ~442). This means the PR's central claim — 'Nothing reads or writes it anymore' — is false in a second, independent way that is invisible to any Python-only investigation: a scheduled background job in a completely different language/runtime/deployment unit depends on this table. RunningDROP TABLE IF EXISTS "posthog_emailreputationsnapshot";while this evaluator is still deployed and scheduled would turn its next Temporal activity run into a hard Postgres error (relation "posthog_emailreputationsnapshot" does not exist), likely producing repeated Temporal activity retries/failures and metric/alerting noise on a completely separate service surface than the one already identified for the Django API endpoint.Suggested fix
Before merging this drop, confirm the Node.js
email-reputation-evaluatorTemporal worker has also been decommissioned or repointed away from this table (not just the Python model/API usage). If this worker is still running in any environment, this migration must wait until it is disabled/removed, exactly as the safe-migrations guide's safety-window requirement intends — but the window needs to cover this Node/Temporal consumer specifically, which the PR description and its stacked prerequisites (#74762/#75237/#75249) never mention.Prompt to fix with AI (copy-paste)