fix(data-imports): retry transient pooler blip on repartition bookkeeping saves - #75747
Conversation
…ping saves Repartition controller bookkeeping writes (partition-size measurement, pending/swap/claim markers) go through `ExternalDataSchema._save_sync_type_config`, an unretried `.save()` fired once per batch across every schema sync. A transient PgBouncer `query_wait_timeout` there was falling straight through to the caller's broad exception handler, silently losing that run's write. Wrap the save with the existing `retry_on_db_connection_drop` helper (already used one call up in the same file for the rollout-flag team lookup) so a one-off pool blip retries once instead of losing the write.
|
😎 This pull request was merged. |
|
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 |
…ave-retry Generated-By: PostHog Code Task-Id: eff56fcd-4f09-40c7-a2b1-7ef7eed9fecd
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, contained reliability fix wrapping an existing DB retry helper around one bookkeeping save; not in risky territory (no schema/API/auth/billing changes), matches an established in-file pattern, and ships with regression tests covering both the retry-succeeds and retry-exhausted paths.
- Author wrote 50% of the modified lines and has 33 merged PRs in these paths (familiarity MODERATE).
- 👍 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 | ✓ | 11L, 1F substantive, 58L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (58L, 2F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ c551d59 · reviewed head 239f7a4 |
1 participant
Problem
Error tracking surfaced an
OperationalError: query_wait_timeout(chained frompsycopg.errors.ProtocolViolation) from the data-imports pipeline, with the top in-app frame inExternalDataSchema.save. It fired during a GoogleAds sync, but the failing code has nothing GoogleAds-specific about it.The call chain:
maybe_flag_for_repartition→schema.record_partition_measurement()→_save_sync_type_config()→schema.save(). This bookkeeping write backs every repartition-controller state update on the schema (partition-size measurement, pending/swap/claim markers, revive markers) and fires once per batch, across every schema sync — a hot-ish path._save_sync_type_configalready skips the activity-log's extraSELECTfor exactly this class of pooler issue (see the comment onsave()), but the.save()call itself had no retry.query_wait_timeoutis PgBouncer signalling that a query waited too long for a free backend connection under pool pressure — a transient infra blip, not a data-shape or config bug.maybe_flag_for_repartitionalready swallows any exception here (detection is deliberately best-effort and must never fail post-load), so the sync itself was never at risk. The gap is that the bookkeeping write for that run is silently lost, and the blip gets reported as an unhandled exception.This mirrors an established pattern already in the same file:
is_auto_repartition_enabled(one function up inrepartition_controller.py) already wraps itsTeam.objects.get()lookup withretry_on_db_connection_dropfor the same reason.Changes
_save_sync_type_confignow wraps its.save()call with the existingretry_on_db_connection_drophelper (posthog/temporal/common/utils.py) — one retry onOperationalError/InterfaceError, evicting the stale pooled connection first. A second consecutive failure still propagates, unchanged.external_data_source.reload_schemas) sotemporaliostays off this model's module-level import path.How did you test this code?
Added
TestSaveSyncTypeConfigRetriesOnConnectionDroptotest_models.py:test_retries_once_on_operational_error_then_succeeds— the first.save()call raisesOperationalError, the second succeeds; asserts the write lands. Catches the regression this fixes: without the retry, the write silently drops on a single pooler blip.test_second_consecutive_operational_error_propagates— guards that a persistent failure still surfaces, rather than being silently retried forever.Ran:
hogli test products/warehouse_sources/backend/tests/test_models.py— 123 passedhogli test products/warehouse_sources/backend/temporal/data_imports/pipelines/core/test_repartition_controller.py— 30 passeduv run mypy --cache-fine-grained products/warehouse_sources/(repo-wide mypy OOM'd in this sandbox — reported viahogli devex:feedback; ran scoped to the touched product instead) — clean, 6152 filesruff check/ruff format— cleantach check --dependencies --interfaces— cleanhogli ci:preflight --fix— 0 failuresAutomatic notifications
Docs update
N/A — internal reliability fix, no user-facing or API behavior change.
🤖 Agent context
Autonomy: Fully autonomous
Triaged directly from the linked error-tracking issue via PostHog's error-tracking MCP tools (
query-error-tracking-issue,query-error-tracking-issue-events), which surfaced the full stack trace and confirmed the failure originates in shared repartition-controller bookkeeping, not the GoogleAds connector. Used Explore subagents to trace the call frequency/concurrency ofmaybe_flag_for_repartition, confirm no existing retry wrapping on this save path, and check for a lighter non-Temporal retry helper before settling on reusingretry_on_db_connection_drop.Checked for a duplicate fix first: searched open PRs by exception type, message phrase, and module path, and reviewed the maintainer's own open PR queue. Found several related-but-non-overlapping PRs applying the same "classify/retry transient PgBouncer blip" pattern to other call sites in this pipeline (#75733, #75735, #75737, #75530) — none touch
_save_sync_type_configor the repartition-controller bookkeeping save path.Invoked
/writing-testsbefore adding the regression test.