fix(data-imports): back off full interval after duckgres enablement refresh failure - #73975
Closed
Gilbert09 wants to merge 1 commit into
Closed
fix(data-imports): back off full interval after duckgres enablement refresh failure#73975Gilbert09 wants to merge 1 commit into
Gilbert09 wants to merge 1 commit into
Conversation
…ment refresh failure The duckgres sink's `_enabled_team_ids` cache only advanced its `_team_ids_fetched_at` timestamp on the very first refresh failure. Once a refresh had succeeded at least once, a later failure left the timestamp stuck in the past, so every subsequent poll tick (~2s) re-attempted the refresh instead of waiting the intended `ENABLEMENT_REFRESH_SECONDS` (60s) window. Now the timestamp always advances on failure, so a transient control-plane outage backs off at the same cadence as a healthy refresh. Generated-By: PostHog Code Task-Id: cd517bae-419a-451a-87b1-f0edaf1c0516
Contributor
|
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 |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
Error tracking surfaced a
RuntimeError("duckgres control plane unreachable; keeping the previous sink enablement") fromproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/duckgres/enablement.py, raised throughDuckgresBatchConsumerAdapter._enabled_team_idsinconsumer.py._enabled_team_idscaches the duckgres-enabled team set and only re-resolves it everyENABLEMENT_REFRESH_SECONDS(60s) — the poll loop itself runs every ~2s. On a refresh failure, theexceptblock correctly keeps the previous team set (so a transient blip doesn't blind the sink), but it only advanced_team_ids_fetched_atwhen this was the very first resolution ever attempted:Once a refresh had already succeeded at least once, a later failure left
_team_ids_fetched_atstuck at its last successful timestamp. Since real time keeps advancing past it, the cache guard (now - self._team_ids_fetched_at < ENABLEMENT_REFRESH_SECONDS) never holds again, so every subsequent poll tick re-attempted the refresh instead of waiting for the intended 60s window — hammering the control plane during an outage and reporting the same error to error tracking roughly every 2 seconds instead of once per minute.The sampled events for this issue all lacked any
warehouse_sources_*sync context, consistent with the failure coming from this background cache refresh rather than any specific source/schema/sync.Changes
consumer.py:_enabled_team_idsnow always advances_team_ids_fetched_aton a refresh failure, not just on the first-ever resolution. The previous team set is still retained on failure (unchanged), but the next retry now waits the fullENABLEMENT_REFRESH_SECONDSinterval instead of firing on every poll tick.How did you test this code?
Added
test_enablement_refresh_failure_after_success_waits_full_interval_before_retryingintest_consumer.py, covering a refresh failure that happens after a prior successful resolution (the case the old code mishandled). It asserts the previous team set is retained and that a second immediate poll does not re-trigger the refresh. Verified it fails against the pre-fix code (2 refresh attempts instead of 1) and passes with the fix.Ran:
uv run pytest products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/duckgres/test_consumer.py -q— 26 passed (3 pre-existing errors inTestLiveApplyStampneed a live Postgres connection and are unaffected by this change)uv run mypy --cache-fine-grained .— clean, 16964 filesruff check/ruff format --check— cleanhogli ci:preflight --fix— 0 failuresDocs update
N/A — internal reliability fix to a background cache refresh, no user-facing or documented behavior change.
🤖 Agent context
Autonomy: Fully autonomous
I (Claude) triaged this from a PostHog error-tracking webhook alert with no human steering the investigation. I pulled the issue's stack trace, impact, and sampled
$exceptionevents via the PostHog MCP error-tracking tools, confirmed all sampled events lackedwarehouse_sources_*sync context (pointing at background/shared code rather than a specific source), then readenablement.pyandconsumer.pyto trace the raise site back through_enabled_team_ids's caching logic. Comparing the refresh-guard condition against the except-block's timestamp update revealed the bug: the timestamp only advances on the very first failure, so a later failure re-triggers on every ~2s poll tick instead of backing off for the full 60s window — which also matches the observed ~2s-apart event cadence in error tracking.Checked for duplicates: searched open PRs by exception message, module path, and function name, and reviewed the maintainer's (Gilbert09) own open PRs. Found two related-but-distinct prior fixes in this same file/class (#73565, #73649) that reclassify timeout exceptions so they're not double-reported to error tracking — neither touches the enablement cache or this retry-cadence bug, so this isn't a duplicate.
Invoked
/writing-testsbefore adding the regression test.Created with PostHog Code