fix(data-imports): retry transient DB connection drop in repartition flag check - #75470
Conversation
…flag check Evict the stale connection and retry once when the Team lookup in `is_auto_repartition_enabled` hits a transient Postgres connection drop, using the existing `retry_on_db_connection_drop` helper. Adds a regression test covering the retry. Generated-By: PostHog Code Task-Id: 6a029cab-6a4a-4099-a438-75ce9612dc6c
|
Hey @Gilbert09! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
There was a problem hiding this comment.
Small, contained fix wrapping one DB read with an existing, documented retry helper for a known transient failure mode; not risky territory, author owns this code with strong familiarity, and a regression test backs the fix.
- Author wrote 100% of the modified lines and has 8 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 3L, 1F substantive, 24L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (24L, 2F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ d24a844 · reviewed head b711fd5 |
fuziontech
left a comment
There was a problem hiding this comment.
Reviewed the complete diff and surrounding retry helper usage. The change correctly retries the Team lookup once on Django connection-drop errors while preserving existing missing-team behavior, and the focused regression test covers the intended failure path. Ruff passes. The focused local pytest could not start because the local Postgres host db is unavailable; the relevant warehouse-sources CI shards passed. No blocking issues found.
This review was generated by an automated review agent on behalf of @fuziontech.
|
/trunk merge |
Problem
Error tracking surfaced an
OperationalError: consuming input failed: server closed the connection unexpectedlyfromis_auto_repartition_enabledinproducts/warehouse_sources/backend/temporal/data_imports/pipelines/core/repartition_controller.py, during a Postgres source sync.The failing line is a plain Django ORM read (
Team.objects.only(...).get(id=schema.team_id)) against PostHog's own app database, not the customer's synced source. It runs viaasyncio.to_threadinside a long-lived Temporal worker thread, so a pooler-recycled or dropped connection surfaces asOperationalErrorthe first time it's reused — a known, transient failure mode in this codebase, not a bug in the sync logic itself.The call is already wrapped in a broad
try/exceptinmaybe_flag_for_repartitionthat reports it viacapture_exceptionand returns without breaking the sync (the function is documented to "never raise" — this is best-effort repartition detection). So the transient blip doesn't break anything, but it does add noise to error tracking on every occurrence, since the dead connection is neither evicted nor retried.Changes
Team.objects.get()read inis_auto_repartition_enabledwithretry_on_db_connection_drop(posthog/temporal/common/utils.py), the existing helper built for exactly this failure class: it evicts the stale pooled connection and retries once onOperationalError/InterfaceError, matching the pattern already used elsewhere in this Temporal worker tree.OperationalErroron the firstTeamlookup and asserts the function retries and resolves the flag instead of the error propagating uncaught.I checked for an existing fix first: #73386 classifies a similar DB-connection blip as transient, but in a different function (
run_pre_write_defensive_compact'sjob.folder_path()resolution) — it doesn't touchis_auto_repartition_enabledor this call site. No open PR covers this specific one.How did you test this code?
Added
TestIsAutoRepartitionEnabled::test_retries_once_on_transient_db_connection_drop, which mocks theTeam.objects.only(...).get()call to raiseOperationalErroronce then succeed, and asserts the function returns the resolved flag value instead of the exception propagating. Without the fix this test fails, since the error isn't caught anywhere insideis_auto_repartition_enabled— it currently escapes to the caller's outer catch-all.Ran:
uv run pytest products/warehouse_sources/backend/temporal/data_imports/pipelines/core/test_repartition_controller.py— 30 passeduv run mypy --cache-fine-grained .(repo-wide, as CI runs it) — clean on the changed filesruff check --fix/ruff format— cleanhogli ci:preflight --fix— cleanAutomatic notifications
Docs update
N/A — internal error-handling change, no user-facing behavior change.
🤖 Agent context
Autonomy: Fully autonomous
Triaged a webhook-delivered error tracking issue using the PostHog MCP error-tracking tools (
query-error-tracking-issue,query-error-tracking-issue-events) to pull the stack trace and confirm the exact failing line and call path. Used a research subagent to check for an existing connection-hygiene pattern in this Temporal worker tree, which surfacedretry_on_db_connection_drop/aretry_on_db_connection_dropinposthog/temporal/common/utils.pyas the established fix for this exact failure class. Checked for duplicate open PRs by exception type, message phrase, module path, and the maintainer's own open PRs before writing the fix — found a related-but-non-overlapping PR (noted above) rather than a duplicate. Invoked/writing-testsbefore adding the regression test.Created with PostHog Code