Skip to content

fix(api): stop transient DB errors from 500ing throttled requests - #76428

Draft
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/fixdb-stop-postgres-catalog-errors-on-e58d44
Draft

fix(api): stop transient DB errors from 500ing throttled requests#76428
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/fixdb-stop-postgres-catalog-errors-on-e58d44

Conversation

@posthog

@posthog posthog Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Problem

  • Users are hitting InternalError: cache lookup failed for function 481 on two request paths, growing fast: 4 occurrences on July 31, 9 on August 1, 134 on August 2.
  • One of the two paths is the DRF throttle chain: is_rate_limit_enabled() reads RATE_LIMIT_ENABLED from get_instance_setting(), but that call sits outside the try/except in allow_request — so a DB error there raises straight through DRF's throttle check into an unhandled 500, instead of the fail-open behavior the rest of the file already uses for this exact class of error.
  • The other reported path (User.objects.exists() in the login_required decorator) is already covered by the separate, unrelated #71723, so it's out of scope here.

Note

The report's other lead — that psycopg3 server-side prepared statements are pinned under PgBouncer because prepare_threshold isn't set — doesn't hold for this codebase. Django's postgresql backend already defaults prepare_threshold to None (disabling it) unless explicitly overridden in OPTIONS, and nothing here sets it. Adding it to posthog/settings/data_stores.py would be a no-op, so I left that out rather than ship a change that doesn't do anything.

Changes

  • Wrap the get_instance_setting("RATE_LIMIT_ENABLED") call in is_rate_limit_enabled in the same try/except-and-fail-open pattern already used elsewhere in posthog/rate_limit.py.
  • Add a rate_limit_fail_open_total Prometheus counter (labeled by scope and exception type) on all three fail-open paths in the file, so a fail-open is now observable instead of silent.

How did you test this code?

Added TestIsRateLimitEnabled in posthog/test/test_rate_limit.py (SimpleTestCase, no DB) asserting is_rate_limit_enabled returns False and reports to error tracking when the underlying instance-setting lookup raises — this is the regression that used to 500 every throttled request during a DB blip.

Ran posthog/test/test_rate_limit.py — the DB-independent cases (including the new one) pass; the DB-backed cases in this environment fail on Postgres connectivity (Name or service not known, no DB reachable in this sandbox), not on this change. ruff check/ruff format --check and mypy pass on both changed files; hogli ci:preflight --fix reports no failures.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Docs update

No user-facing docs affected — this is an internal resilience fix with no behavior change to the rate-limiting product surface.

🤖 Agent context

Autonomy: Fully autonomous

Authored by PostHog Code from a PostHog inbox report about accelerating cache lookup failed for function 481 Postgres errors on two request paths. Checked first for existing overlapping work: found #71723 (caches the login_required users-exist check, the other path in the report) but nothing touching posthog/rate_limit.py's exception handling or posthog/settings/data_stores.py's prepare_threshold.

Investigated the report's prepare_threshold hypothesis by reading Django's postgresql backend (get_connection_params) and confirmed it already defaults prepare_threshold=None for psycopg3 — so that half of the suggested fix would have been a no-op, and I didn't make it.

Traced the throttle path further than the report did: the actual gap isn't the existing except Exception: return True blocks (those were already correctly catching and fail-opening), it's that is_rate_limit_enabled() is called before any try/except in allow_request, so it wasn't being caught at all — it would 500 the request rather than silently skip rate limiting. Fixed that specific gap and added a metric so all three fail-open paths are observable going forward.

Invoked /writing-tests before adding the regression test.


Created with PostHog Desktop from this inbox report.

The `RATE_LIMIT_ENABLED` instance-setting lookup in `is_rate_limit_enabled` runs outside any try/except in `allow_request`, so a Postgres blip there raises straight through DRF's throttle check instead of falling back to the fail-open behavior the rest of the file already uses. Wrap it the same way, and add a counter so every fail-open (this one plus the two existing ones) is observable instead of silent.

Generated-By: PostHog Code
Task-Id: eb559adc-e2b6-4c94-80c9-5d0c358cc11e
@trunk-io

trunk-io Bot commented Aug 2, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants