fix(postgres): surface leaked transaction nesting as retryable during setup - #70293
Conversation
… setup A dropped connection while psycopg entered/left a metadata-probe transaction() block can leave the transaction-nesting counter incremented while `connection.broken` stays False (the connection is INERROR, not BAD). The setup guard only checked `broken`, so it missed this case and the implicit `with connection:` exit-commit raised a misleading `ProgrammingError: Explicit commit() forbidden within a Transaction context`, burying the real, transient cause. Broaden the guard to also fire on a leaked nesting counter and surface the retryable dropped-connection error, so the activity retries on a fresh connection. Generated-By: PostHog Code Task-Id: c46319a8-fc65-48aa-8040-c26c5186371e
|
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 |
|
Reviews (1): Last reviewed commit: "fix(postgres): surface leaked transactio..." | Re-trigger Greptile |
There was a problem hiding this comment.
Small, well-tested fix to a retry-detection guard in the Postgres warehouse source, by the owning-team author with strong familiarity; no risky territory, no unresolved concerns, and a bot reviewer reaction is positive.
- Author wrote 100% of the modified lines and has 38 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 14L, 1F substantive, 33L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (33L, 2F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ dc71029 · reviewed head 551a5b0 |
Problem
Error tracking surfaced a Postgres data-warehouse import failing with:
The real stack originates in the Postgres source's metadata-discovery setup (
postgres_source→source_for_pipeline), and the failing operation is the implicitcommit()that psycopg runs when thewith connection:setup block exits successfully. The connection in the captured event is alive butINERROR, not broken.Root cause: during setup we run best-effort probes, one of which isolates its query in a
connection.transaction()block. psycopg increments a transaction-nesting counter on enter and only decrements it on exit when the connection is still OK. A connection dropped precisely while entering or leaving that block can leave the counter incremented without flippingconnection.broken(the connection readsINERROR, notBAD). psycopg'scommit()refuses outright whenever that counter is non-zero, so the exit-commit raises the misleadingProgrammingErrorand buries the real, transient cause.There is already a guard (
_raise_if_setup_connection_broken) meant to convert exactly this masked error into a retryable dropped-connection error. It only checkedconnection.broken, so it missed the not-broken-but-counter-leaked case.Error tracking issue: https://us.posthog.com/project/2/error_tracking/019f5687-17a3-7e81-b64c-07904c6b4fca
Changes
Broaden
_raise_if_setup_connection_brokento fire on either signal: a broken connection or a leaked transaction-nesting counter (_num_transactions > 0). The counter is the exact predicate psycopg'scommit()checks, so this preempts the misleading error precisely.The raised error is the same retryable
OperationalErrorthe broken-connection path already used, so it's classified as a transient drop (matches_is_connection_dropped_error, matches noNonRetryableErrorssubstring) and the activity retries on a fresh connection instead of failing on a self-inflicted commit error.Note
This is deliberately kept retryable. A statement error inside the probe (e.g. a
statement_timeout) tears its transaction down cleanly and does not leak the counter, so this change does not turn genuine query failures into infinite retries — the counter only leaks on a connection-level problem mid-probe, which is transient.How did you test this code?
Automated only. I (Claude) added a regression test and ran the guard's unit tests locally with the repo venv:
New test
test_leaked_transaction_counter_raises_retryable_dropped_errorreproduces the production state — a non-broken connection (broken = False) with a leaked counter (_num_transactions = 1) — and asserts the guard raises a retryable error not matched by anyNonRetryableErrorssubstring. No existing test covered the not-broken-but-counter-leaked path; the existing tests only coveredbroken = Trueand a fully healthy connection. ruff check and format are clean.Automatic notifications
🤖 Agent context
Autonomy: Fully autonomous
Triaged from an error-tracking webhook. I (Claude) pulled the issue's stack trace and a representative event via the PostHog MCP error-tracking tools, confirmed the failure genuinely originates in the Postgres source setup (not just serialized log context), and read psycopg's
transaction.py/_connection_base.pyto pin down thatcommit()gates purely on the transaction-nesting counter and thatTransaction.__exit__skips the decrement when the connection is not OK.Decision: this is a fragile-code bug, not a user/upstream error, so I fixed the code rather than extending
NonRetryableErrors. The maskedProgrammingErrorhides a transient dropped-connection condition that should stay retryable, so the fix surfaces the real retryable error rather than suppressing it. Scope kept to the existing guard; no retry-policy or unrelated changes.Skills invoked:
/writing-tests(before adding the regression test).Created with PostHog Code