fix(growth): recycle dead db connections in signup enrichment worker - #72307
Conversation
The activity called close_old_connections() on the event loop thread, but Django connections are thread-local and all ORM work runs on asgiref's shared sync-executor thread. A dropped Postgres connection in that thread was never closed, so every subsequent activity (including all retries) failed until the pod was replaced. Run the cleanup through sync_to_async so it executes on the thread that owns the connections. Also adds a backfill_signup_enrichment management command to re-dispatch enrichment for orgs whose runs failed (eligible at signup, no archived provider fetch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Swibx35joPjMxqmSBLmb8
… command Use the canonical @close_db_connections decorator from temporal common utils instead of a hand-rolled sync_to_async cleanup call, add a US region guard, queryset iterator, and dispatch-counted --limit to the backfill command, make domain_from_email public, and drop unneeded __init__.py files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Swibx35joPjMxqmSBLmb8
…emails Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Swibx35joPjMxqmSBLmb8
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/growth/backend/management/commands/backfill_signup_enrichment.py:62-68
**Current Member Replaces Signup Identity**
When the original signup user has left the organization, the earliest remaining membership can belong to another person. The command then enriches using that member's email domain and `distinct_id`, which can merge data for the wrong company into the organization and attribute events to the wrong person.
### Issue 2 of 2
products/growth/backend/management/commands/backfill_signup_enrichment.py:53
**Best-Effort Archive Marks Completion**
`archive_provider_fetch` suppresses archive-write failures and allows the enrichment result to be persisted afterward. A successfully enriched organization can therefore have no fetch row, match this query, and be enriched again, causing duplicate provider requests and repeated writes.
Reviews (1): Last reviewed commit: "fix(growth): propagate backfill dispatch..." | Re-trigger Greptile |
…window membership Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Swibx35joPjMxqmSBLmb8
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/growth/backend/management/commands/backfill_signup_enrichment.py |
88.4% | 49, 54, 74, 90–92, 118–119 |
🤖 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 29850775545 -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.6% |
8,414 / 21,244 |
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
warehouse_sources_queue |
████████████░░░░░░░░ 59.2% |
148 / 250 |
tasks |
█████████████░░░░░░░ 66.4% |
26,566 / 40,009 |
data_tools |
██████████████░░░░░░ 70.0% |
63 / 90 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
████████████████░░░░ 79.0% |
20,108 / 25,439 |
data_modeling |
████████████████░░░░ 80.0% |
4,834 / 6,045 |
cdp |
████████████████░░░░ 80.7% |
3,118 / 3,864 |
wizard |
█████████████████░░░ 84.8% |
935 / 1,102 |
notebooks |
█████████████████░░░ 85.3% |
7,266 / 8,522 |
agent_platform |
█████████████████░░░ 86.4% |
3,807 / 4,405 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
cohorts |
█████████████████░░░ 87.0% |
4,303 / 4,945 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
exports |
██████████████████░░ 88.4% |
6,943 / 7,853 |
business_knowledge |
██████████████████░░ 89.0% |
4,391 / 4,936 |
conversations |
██████████████████░░ 89.2% |
16,508 / 18,509 |
dashboards |
██████████████████░░ 89.3% |
5,928 / 6,640 |
visual_review |
██████████████████░░ 89.4% |
5,818 / 6,505 |
engineering_analytics |
██████████████████░░ 89.5% |
5,507 / 6,154 |
mcp_analytics |
██████████████████░░ 89.7% |
2,663 / 2,969 |
alerts |
██████████████████░░ 89.9% |
4,054 / 4,508 |
error_tracking |
██████████████████░░ 90.1% |
9,761 / 10,832 |
early_access_features |
██████████████████░░ 90.1% |
1,031 / 1,144 |
streamlit_apps |
██████████████████░░ 90.4% |
2,501 / 2,767 |
slack_app |
██████████████████░░ 90.6% |
8,989 / 9,926 |
links |
██████████████████░░ 90.6% |
183 / 202 |
marketing_analytics |
██████████████████░░ 90.8% |
11,514 / 12,684 |
stamphog |
██████████████████░░ 91.0% |
3,993 / 4,387 |
product_analytics |
██████████████████░░ 91.3% |
5,786 / 6,337 |
mcp_store |
██████████████████░░ 92.0% |
4,261 / 4,634 |
data_warehouse |
███████████████████░ 92.6% |
19,175 / 20,715 |
ai_observability |
███████████████████░ 92.7% |
14,758 / 15,926 |
notifications |
███████████████████░ 92.7% |
1,031 / 1,112 |
web_analytics |
███████████████████░ 92.9% |
13,853 / 14,913 |
workflows |
███████████████████░ 92.9% |
5,648 / 6,077 |
surveys |
███████████████████░ 93.0% |
5,724 / 6,157 |
posthog_ai |
███████████████████░ 93.2% |
1,325 / 1,421 |
approvals |
███████████████████░ 93.3% |
3,395 / 3,640 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
managed_migrations |
███████████████████░ 93.8% |
1,220 / 1,300 |
tracing |
███████████████████░ 93.9% |
2,635 / 2,805 |
legal_documents |
███████████████████░ 94.1% |
1,568 / 1,667 |
endpoints |
███████████████████░ 94.1% |
8,606 / 9,143 |
messaging |
███████████████████░ 94.3% |
2,682 / 2,845 |
revenue_analytics |
███████████████████░ 94.5% |
3,598 / 3,809 |
skills |
███████████████████░ 94.5% |
2,893 / 3,061 |
review_hog |
███████████████████░ 94.6% |
6,808 / 7,193 |
logs |
███████████████████░ 95.4% |
9,935 / 10,416 |
experiments |
███████████████████░ 95.7% |
24,469 / 25,569 |
growth |
███████████████████░ 95.8% |
2,952 / 3,083 |
replay_vision |
███████████████████░ 95.8% |
13,874 / 14,481 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
feature_flags |
███████████████████░ 96.3% |
16,311 / 16,940 |
user_interviews |
███████████████████░ 96.5% |
2,639 / 2,735 |
warehouse_sources |
███████████████████░ 96.8% |
299,834 / 309,891 |
access_control |
███████████████████░ 96.8% |
849 / 877 |
customer_analytics |
███████████████████░ 97.3% |
7,780 / 7,999 |
data_catalog |
███████████████████░ 97.5% |
2,346 / 2,407 |
analytics_platform |
████████████████████ 98.0% |
2,102 / 2,145 |
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.
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 2 consider. Published 3 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
| def dispatch_signup_enrichment(inputs: SignupEnrichmentInputs) -> None: | ||
| """Synchronous dispatch for operational re-runs (management commands). | ||
|
|
||
| Skips the signup-path guards on purpose: the operator has already selected the orgs, | ||
| so the kill switch, region gate, and bounded pool don't apply. Unlike the | ||
| fire-and-forget signup path, dispatch errors propagate so the operator sees them; | ||
| a still-running workflow for the org counts as dispatched. | ||
| """ | ||
| try: | ||
| _start_workflow(inputs) | ||
| except WorkflowAlreadyStartedError: | ||
| logger.info("signup_enrichment_dispatch_skipped", organization_id=inputs.organization_id) | ||
|
|
||
|
|
There was a problem hiding this comment.
Backfill dispatch bypasses the signup-enrichment kill switch
Why we think it's a valid issue
- Checked: Grepped every production reference to
GROWTH_SIGNUP_ENRICHMENT_ENABLED, read the full dispatch module (trigger.py), the worker activity (workflow.py), and the backfill command'shandle(). - Found: The flag is enforced in exactly one production location —
trigger.py:58insidestart_signup_enrichment_workflow. The worker activityenrich_signup_organization_activity(workflow.py:63-159) never re-checks it before instantiatingHarmonicEnrichmentProvider()and calling out to Harmonic. So the dispatch-time check is the sole enforcement point of the kill switch — there is no second line of defense on the worker. - Found:
dispatch_signup_enrichment(trigger.py:81-93) calls_start_workflowdirectly with no flag check, and the command (backfill_signup_enrichment.py:41-64) re-implements only the US-only region gate as aCommandError, not the kill switch. - Found (asymmetry): the
dispatch_signup_enrichmentdocstring claims the region gate "don't apply," yet the command explicitly re-adds it (backfill_signup_enrichment.py:42-44). The author's own logic thus already re-applies a global feature guard for the operator path; the kill switch is the one global guard left unguarded, and it governs whether the third-party (Harmonic) integration is active at all — orthogonal to the "operator already selected the orgs" rationale used to justify skipping it. - Impact: The flag defaults to
False(settings/web.py:353) and doubles as the master on/off for sending org domain/headcount data to Harmonic. If it is flipped off (incident, vendor/contract, or cost reason) after launch, running the backfill still dispatches real workflows and the worker calls Harmonic for every matching org, silently defeating the switch. Concrete trigger and concrete consequence are both nameable; the fix (a one-lineCommandErrorguard mirroring the region check) is small — meets the keep bar as a reliability/data-governance control gap.
Issue description
dispatch_signup_enrichment (used only by the new backfill_signup_enrichment command) starts the Temporal workflow via _start_workflow without checking settings.GROWTH_SIGNUP_ENRICHMENT_ENABLED, unlike start_signup_enrichment_workflow which gates on that flag first. The docstring calls this out as intentional ("the kill switch, region gate, and bounded pool don't apply"), but the command only re-implements the region gate (backfill_signup_enrichment.py lines 42-44), not the kill switch. GROWTH_SIGNUP_ENRICHMENT_ENABLED is the master control for whether organization domain/headcount data is sent to the third-party Harmonic provider; if it is ever turned off for a compliance, cost, or vendor-contract reason, this operational entry point still dispatches real enrichment workflows and still calls out to Harmonic for every matching org, silently defeating the switch.
Suggested fix
Have dispatch_signup_enrichment (or the command's handle()) check settings.GROWTH_SIGNUP_ENRICHMENT_ENABLED and refuse to dispatch (raise CommandError, mirroring the existing US-only guard) when the feature is disabled, so the kill switch stays authoritative across both dispatch paths.
Prompt to fix with AI (copy-paste)
## Context
@posthog/temporal/signup_enrichment/trigger.py#L81-94
<issue_description>
`dispatch_signup_enrichment` (used only by the new `backfill_signup_enrichment` command) starts the Temporal workflow via `_start_workflow` without checking `settings.GROWTH_SIGNUP_ENRICHMENT_ENABLED`, unlike `start_signup_enrichment_workflow` which gates on that flag first. The docstring calls this out as intentional ("the kill switch, region gate, and bounded pool don't apply"), but the command only re-implements the region gate (backfill_signup_enrichment.py lines 42-44), not the kill switch. `GROWTH_SIGNUP_ENRICHMENT_ENABLED` is the master control for whether organization domain/headcount data is sent to the third-party Harmonic provider; if it is ever turned off for a compliance, cost, or vendor-contract reason, this operational entry point still dispatches real enrichment workflows and still calls out to Harmonic for every matching org, silently defeating the switch.
</issue_description>
<issue_validation>
- **Checked:** Grepped every production reference to `GROWTH_SIGNUP_ENRICHMENT_ENABLED`, read the full dispatch module (`trigger.py`), the worker activity (`workflow.py`), and the backfill command's `handle()`.
- **Found:** The flag is enforced in exactly one production location — `trigger.py:58` inside `start_signup_enrichment_workflow`. The worker activity `enrich_signup_organization_activity` (`workflow.py:63-159`) never re-checks it before instantiating `HarmonicEnrichmentProvider()` and calling out to Harmonic. So the dispatch-time check is the *sole* enforcement point of the kill switch — there is no second line of defense on the worker.
- **Found:** `dispatch_signup_enrichment` (`trigger.py:81-93`) calls `_start_workflow` directly with no flag check, and the command (`backfill_signup_enrichment.py:41-64`) re-implements only the US-only region gate as a `CommandError`, not the kill switch.
- **Found (asymmetry):** the `dispatch_signup_enrichment` docstring claims the region gate "don't apply," yet the command explicitly re-adds it (`backfill_signup_enrichment.py:42-44`). The author's own logic thus already re-applies a global feature guard for the operator path; the kill switch is the one global guard left unguarded, and it governs whether the third-party (Harmonic) integration is active at all — orthogonal to the "operator already selected the orgs" rationale used to justify skipping it.
- **Impact:** The flag defaults to `False` (`settings/web.py:353`) and doubles as the master on/off for sending org domain/headcount data to Harmonic. If it is flipped off (incident, vendor/contract, or cost reason) after launch, running the backfill still dispatches real workflows and the worker calls Harmonic for every matching org, silently defeating the switch. Concrete trigger and concrete consequence are both nameable; the fix (a one-line `CommandError` guard mirroring the region check) is small — meets the keep bar as a reliability/data-governance control gap.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Have `dispatch_signup_enrichment` (or the command's `handle()`) check `settings.GROWTH_SIGNUP_ENRICHMENT_ENABLED` and refuse to dispatch (raise `CommandError`, mirroring the existing US-only guard) when the feature is disabled, so the kill switch stays authoritative across both dispatch paths.
</potential_solution>
There was a problem hiding this comment.
Valid. Command now raises CommandError when GROWTH_SIGNUP_ENRICHMENT_ENABLED is off, mirroring the region guard, and the dispatch docstring says callers own both gates (cd5d292).
| if options["dry_run"]: | ||
| self.stdout.write(f"would dispatch {org.id} ({org.created_at:%Y-%m-%d %H:%M}) domain={domain}") | ||
| else: | ||
| dispatch_signup_enrichment(inputs) | ||
| self.stdout.write(f"dispatched {org.id} ({org.created_at:%Y-%m-%d %H:%M}) domain={domain}") | ||
| time.sleep(options["delay"]) | ||
| dispatched += 1 |
There was a problem hiding this comment.
No per-organization error isolation in the backfill dispatch loop
Why we think it's a valid issue
- Checked:
dispatch_signup_enrichmentexception handling (trigger.py:89-92), the command loop (backfill_signup_enrichment.py:67-96), the queryset exclusions (lines 52-64), and re-dispatch idempotency. - Found: The premise is accurate — only
WorkflowAlreadyStartedErroris caught (trigger.py:91);RPCErrorand any other exception propagate, and the loop wraps thedispatch_signup_enrichment(inputs)call in no try/except (lines 92-95), so the first non-WASerror aborts the whole run. The docstring's "errors propagate so the operator sees them" (trigger.py:86) is written for single-org operational use; in the batch loop it means one transient hiccup ends the batch. - Found (mitigating): The command is idempotent and self-resumable. The queryset excludes orgs that already have a fetch or snapshot row (lines 58-61), and re-dispatch is safe (deterministic workflow id +
ALLOW_DUPLICATE, withWorkflowAlreadyStartedErrorswallowed). After an abort, re-running the same window skips completed orgs and harmlessly re-dispatches incomplete/unattempted ones — no lost work, no corruption, no wrong data. The failure is also loud (traceback) with the last-processed org visible via per-org stdout (line 94), so the operator sees where it stopped. - Impact: A transient Temporal/RPC error during outage recovery is a plausible failure mode with no in-loop handling, and continue-on-error + an error tally is a standard, low-cost batch-command pattern — so this is a genuine reliability/ergonomics gap, not noise, wrong, or an unreachable edge case.
- Priority: Downgrade to
consider— the idempotent, resumable, loud-failure behavior reduces the real consequence to "operator re-runs the command," well short of the should_fix framing, which overstates it as leaving orgs silently unattended.
Issue description
dispatch_signup_enrichment (trigger.py:81-94) only swallows WorkflowAlreadyStartedError; every other exception (a transient Temporal RPC error, a dropped DB connection while reading user.email/user.distinct_id, etc.) propagates straight out of the for org in orgs: loop and kills the whole management-command run. For a command whose entire purpose is to recover from an infrastructure outage, aborting the batch on the first transient error after processing only some of the window — with no per-org error tally and no indication in the summary line of how many orgs were never attempted — is a real operational-reliability gap. The --limit/--dry-run counters (dispatched, skipped) give no visibility into this failure mode either.
Suggested fix
Wrap the dispatch_signup_enrichment(inputs) call in a try/except that logs the org id and error, increments an errored counter, and continues to the next org, then report errored alongside dispatched/skipped in the final summary line so the operator knows exactly which orgs still need a re-run.
Prompt to fix with AI (copy-paste)
## Context
@products/growth/backend/management/commands/backfill_signup_enrichment.py#L90-96
<issue_description>
`dispatch_signup_enrichment` (trigger.py:81-94) only swallows `WorkflowAlreadyStartedError`; every other exception (a transient Temporal RPC error, a dropped DB connection while reading `user.email`/`user.distinct_id`, etc.) propagates straight out of the `for org in orgs:` loop and kills the whole management-command run. For a command whose entire purpose is to recover from an infrastructure outage, aborting the batch on the first transient error after processing only some of the window — with no per-org error tally and no indication in the summary line of how many orgs were never attempted — is a real operational-reliability gap. The `--limit`/`--dry-run` counters (`dispatched`, `skipped`) give no visibility into this failure mode either.
</issue_description>
<issue_validation>
- **Checked:** `dispatch_signup_enrichment` exception handling (`trigger.py:89-92`), the command loop (`backfill_signup_enrichment.py:67-96`), the queryset exclusions (lines 52-64), and re-dispatch idempotency.
- **Found:** The premise is accurate — only `WorkflowAlreadyStartedError` is caught (`trigger.py:91`); `RPCError` and any other exception propagate, and the loop wraps the `dispatch_signup_enrichment(inputs)` call in no try/except (lines 92-95), so the first non-`WAS` error aborts the whole run. The docstring's "errors propagate so the operator sees them" (trigger.py:86) is written for *single-org* operational use; in the batch loop it means one transient hiccup ends the batch.
- **Found (mitigating):** The command is idempotent and self-resumable. The queryset excludes orgs that already have a fetch or snapshot row (lines 58-61), and re-dispatch is safe (deterministic workflow id + `ALLOW_DUPLICATE`, with `WorkflowAlreadyStartedError` swallowed). After an abort, re-running the same window skips completed orgs and harmlessly re-dispatches incomplete/unattempted ones — no lost work, no corruption, no wrong data. The failure is also loud (traceback) with the last-processed org visible via per-org stdout (line 94), so the operator sees where it stopped.
- **Impact:** A transient Temporal/RPC error during outage recovery is a plausible failure mode with no in-loop handling, and continue-on-error + an error tally is a standard, low-cost batch-command pattern — so this is a genuine reliability/ergonomics gap, not noise, wrong, or an unreachable edge case.
- **Priority:** Downgrade to `consider` — the idempotent, resumable, loud-failure behavior reduces the real consequence to "operator re-runs the command," well short of the should_fix framing, which overstates it as leaving orgs silently unattended.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Wrap the `dispatch_signup_enrichment(inputs)` call in a try/except that logs the org id and error, increments an `errored` counter, and continues to the next org, then report `errored` alongside `dispatched`/`skipped` in the final summary line so the operator knows exactly which orgs still need a re-run.
</potential_solution>
There was a problem hiding this comment.
Valid. The loop now catches per-org dispatch errors, prints them to stderr, reports an errored count in the summary, and continues (cd5d292).
| def dispatch_signup_enrichment(inputs: SignupEnrichmentInputs) -> None: | ||
| """Synchronous dispatch for operational re-runs (management commands). | ||
|
|
||
| Skips the signup-path guards on purpose: the operator has already selected the orgs, | ||
| so the kill switch, region gate, and bounded pool don't apply. Unlike the | ||
| fire-and-forget signup path, dispatch errors propagate so the operator sees them; | ||
| a still-running workflow for the org counts as dispatched. | ||
| """ | ||
| try: | ||
| _start_workflow(inputs) | ||
| except WorkflowAlreadyStartedError: | ||
| logger.info("signup_enrichment_dispatch_skipped", organization_id=inputs.organization_id) | ||
|
|
||
|
|
There was a problem hiding this comment.
New dispatch_signup_enrichment has no test covering its documented error-propagation contract
Why we think it's a valid issue
- Checked: The two sibling functions' except clauses (
trigger.py:89-92swallows onlyWorkflowAlreadyStartedError;_dispatchattrigger.py:138-148also swallowsRPCError+ genericException), the command test, and all oftest_trigger.py. - Found: The claim holds —
dispatch_signup_enrichment's body is never executed by any test. The command test patches it out entirely (test_enrichment_backfill_command.py:54,68), and every one of the 12test_trigger.pytests drivesstart_signup_enrichment_workflow, notdispatch_signup_enrichment(grep confirms zero references). In particulartest_duplicate_workflow_is_logged_not_captured(line 121) exercises the signup path's_dispatch(the swallow-everything sibling), so it does not cover the new function's divergent handling. - Found: The divergent error-propagation behavior is the function's documented reason for existing (docstring lines 85-87), yet has zero coverage — and the regression is nameable and observable: widening the except clause (or a copy-paste from the adjacent
_dispatch) would silently swallow dispatch failures, so the backfill would print "dispatched" for orgs that actually failed. No existing test catches this, and the assertion (does the exception propagate out of the public function) is behavioral, not implementation-pinning — so it clears the testing value gate. - Impact: A real, cheap-to-close coverage gap on a new public entry point's stated contract — not noise or a change-detector-for-implementation-detail.
- Priority: Downgrade should_fix → consider. The function is 4 trivial lines, the feared regression is a hypothetical future edit already guarded by an explicit docstring, and the consequence is mild (a one-off operator command losing dispatch-error visibility — not user-facing, no data loss).
Issue description
dispatch_signup_enrichment is a brand-new public entry point whose entire reason for existing (per its own docstring) is that it behaves differently from the sibling _dispatch used by the signup path: it only swallows WorkflowAlreadyStartedError, letting RPCError and any other Exception propagate to the operator instead of being logged-and-swallowed like _dispatch does (compare the except clauses at lines 91-92 vs. 141-148). Every existing test that touches this function (test_enrichment_backfill_command.py) mocks dispatch_signup_enrichment out completely, so nothing in the suite ever calls the real function body. There is no test asserting that a WorkflowAlreadyStartedError is caught, and — more importantly — none asserting that a generic Exception/RPCError from _start_workflow is NOT caught and instead propagates. Since _dispatch sits right above/below it with a near-identical try block but broader exception handling, a future edit (e.g. someone 'fixing' what looks like an inconsistency, or a copy-paste from _dispatch) could silently widen the except clause and defeat the whole point of this function, and CI would not catch it.
Suggested fix
Add a focused unit test in posthog/temporal/tests/signup_enrichment/test_trigger.py (mirroring the existing sync_connect/asyncio.run mocking pattern already used for start_signup_enrichment_workflow) that calls dispatch_signup_enrichment directly: one case where _start_workflow raises WorkflowAlreadyStartedError and asserts it is swallowed/logged, and one case where it raises a plain Exception (or RPCError) and asserts it propagates out of dispatch_signup_enrichment uncaught.
Prompt to fix with AI (copy-paste)
## Context
@posthog/temporal/signup_enrichment/trigger.py#L81-94
@posthog/temporal/signup_enrichment/trigger.py#L138-148
<issue_description>
`dispatch_signup_enrichment` is a brand-new public entry point whose entire reason for existing (per its own docstring) is that it behaves differently from the sibling `_dispatch` used by the signup path: it only swallows `WorkflowAlreadyStartedError`, letting `RPCError` and any other `Exception` propagate to the operator instead of being logged-and-swallowed like `_dispatch` does (compare the except clauses at lines 91-92 vs. 141-148). Every existing test that touches this function (`test_enrichment_backfill_command.py`) mocks `dispatch_signup_enrichment` out completely, so nothing in the suite ever calls the real function body. There is no test asserting that a `WorkflowAlreadyStartedError` is caught, and — more importantly — none asserting that a generic `Exception`/`RPCError` from `_start_workflow` is NOT caught and instead propagates. Since `_dispatch` sits right above/below it with a near-identical try block but broader exception handling, a future edit (e.g. someone 'fixing' what looks like an inconsistency, or a copy-paste from `_dispatch`) could silently widen the except clause and defeat the whole point of this function, and CI would not catch it.
</issue_description>
<issue_validation>
- **Checked:** The two sibling functions' except clauses (`trigger.py:89-92` swallows only `WorkflowAlreadyStartedError`; `_dispatch` at `trigger.py:138-148` also swallows `RPCError` + generic `Exception`), the command test, and all of `test_trigger.py`.
- **Found:** The claim holds — `dispatch_signup_enrichment`'s body is never executed by any test. The command test patches it out entirely (`test_enrichment_backfill_command.py:54,68`), and every one of the 12 `test_trigger.py` tests drives `start_signup_enrichment_workflow`, not `dispatch_signup_enrichment` (grep confirms zero references). In particular `test_duplicate_workflow_is_logged_not_captured` (line 121) exercises the *signup* path's `_dispatch` (the swallow-everything sibling), so it does not cover the new function's divergent handling.
- **Found:** The divergent error-propagation behavior is the function's documented reason for existing (docstring lines 85-87), yet has zero coverage — and the regression is nameable and observable: widening the except clause (or a copy-paste from the adjacent `_dispatch`) would silently swallow dispatch failures, so the backfill would print "dispatched" for orgs that actually failed. No existing test catches this, and the assertion (does the exception propagate out of the public function) is behavioral, not implementation-pinning — so it clears the testing value gate.
- **Impact:** A real, cheap-to-close coverage gap on a new public entry point's stated contract — not noise or a change-detector-for-implementation-detail.
- **Priority:** Downgrade should_fix → consider. The function is 4 trivial lines, the feared regression is a hypothetical future edit already guarded by an explicit docstring, and the consequence is mild (a one-off operator command losing dispatch-error visibility — not user-facing, no data loss).
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add a focused unit test in `posthog/temporal/tests/signup_enrichment/test_trigger.py` (mirroring the existing `sync_connect`/`asyncio.run` mocking pattern already used for `start_signup_enrichment_workflow`) that calls `dispatch_signup_enrichment` directly: one case where `_start_workflow` raises `WorkflowAlreadyStartedError` and asserts it is swallowed/logged, and one case where it raises a plain `Exception` (or `RPCError`) and asserts it propagates out of `dispatch_signup_enrichment` uncaught.
</potential_solution>
There was a problem hiding this comment.
Valid. Added a parametrized test driving the real function: WorkflowAlreadyStartedError is swallowed, RuntimeError propagates (cd5d292).
…atch errors Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Swibx35joPjMxqmSBLmb8
Problem
The signup enrichment worker called
close_old_connections()at the start of each activity, but the activity is async: the call ran on the event loop thread while every ORM call runs throughsync_to_asyncon asgiref's shared sync-executor thread. Django connections are thread-local, so the cleanup inspected an empty connection list and the executor thread's connection was never recycled.When that thread's Postgres connection dropped, the worker could not recover: every subsequent activity, including all retries, failed with
OperationalError: the connection is lost / the connection is closeduntil the pod was replaced by a deploy. This took signup enrichment fully down for ~27 hours (Jul 19 04:40 UTC to Jul 20 08:10 UTC), caught by thesignup_enrichment_completedfailure alert; recovery came from an unrelated rolling deploy. Context: https://posthog.slack.com/archives/C091NMMUT0U/p1784553965935759Changes
close_old_connections()call with the canonical@close_db_connectionsdecorator fromposthog/temporal/common/utils.py(stacked under@activity.defn, as ~20 other activities do). Its async branch runs cleanup throughsync_to_async, i.e. on the thread that owns the connections, before and after each attempt, so a dead connection now recovers on the next retry instead of persisting for the pod's lifetime.backfill_signup_enrichmentmanagement command that re-dispatches enrichment for orgs created in a window that were eligible at signup (work email recorded) but have no archived provider fetch, i.e. their run never completed. US-only guard mirroring the signup path,--dry-run,--limit(counts dispatches),--delay, and an iterator over the queryset. This covers the orgs affected by the outage.dispatch_signup_enrichment(dispatch errors propagate to the operator, unlike the fire-and-forget signup path) and makedomain_from_emailpublic intrigger.pyfor the command. The command also re-checks the member email againstGenericEmailssince the earliest member can have drifted since signup.Note for a follow-up sweep: the same loop-thread
close_old_connections()pattern exists in other async activities (posthog/temporal/salesforce_enrichment/*,posthog/temporal/experiments/activities.py,posthog/temporal/session_replay/rasterize_recording/activities/rasterize.py). Left untouched here to keep the diff scoped.How did you test this code
posthog/temporal/tests/signup_enrichment/suite passes locally; repo-wide mypy shows no new errors in changed files.The thread-identity assertion is deliberate: the bug's only in-process signature is which thread cleanup runs on, so the test pins that contract rather than implementation choreography.
Changelog
no
🤖 Agent context
Diagnosed from production logs: all incident-window errors came from one pod's shared asgiref sync-executor thread, and a
close_old_connections()call at the top of the activity predated this fix but ran on the event loop thread. asgirefthread_sensitive=Trueexecutor semantics were verified against the vendored asgiref 3.11.0 source.