fix(data-imports): classify DB connection blips as transient in pre-write maintenance - #73386
Conversation
|
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.
Narrow widening of an already-existing transient-error classifier in a warehouse-sync maintenance path, backed by regression tests; author owns this area with strong familiarity and no risky territory (no migration, API, auth, billing, or CI changes) is touched.
- Author wrote 100% of the modified lines and has 40 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 | ✓ | 24L, 2F substantive, 50L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (50L, 3F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 4ac650f · reviewed head cc2d38e |
|
😎 Merged manually by talyn-app[bot] - details. |
cc2d38e to
abb0751
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Small, well-tested error-classification change in an internal warehouse-sync maintenance path (already wrapped in try/except, doesn't affect the actual sync); not risky territory, and author has strong familiarity with these exact files.
- Author wrote 100% of the modified lines and has 15 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 | ✓ | 25L, 2F substantive, 51L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (51L, 3F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 7c4151e · reviewed head abb0751 |
abb0751 to
0801005
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
|
/trunk merge |
There was a problem hiding this comment.
Small, well-tested broadening of an existing transient-error classifier to also cover DB connection blips, consistent with established precedent elsewhere in the same pipeline; no risky-territory surface touched and author has strong ownership/familiarity.
- Author wrote 100% of the modified lines and has 22 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 | ✓ | 29L, 2F substantive, 55L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (55L, 3F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ c033211 · reviewed head 0801005 |
|
Heads-up on the red The The wheel republish is already in flight in #75455 ( This PR itself only touches 🦉 via talyn.dev |
|
/trunk merge |
6a98cb6 to
dcf80ed
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Small, well-tested broadening of a transient-error classifier in a background maintenance retry path; no data model, API, auth, billing, CI, or dependency surface touched, and the author (owning team, STRONG familiarity) covers it with a regression test matching the described bug.
- Author wrote 100% of the modified lines and has 25 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 | ✓ | 29L, 2F substantive, 55L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (55L, 3F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ ec35a5a · reviewed head dcf80ed |
dcf80ed to
54409f3
Compare
|
Status update — the red CI here is a master-side regression, not this PR. This PR only touches What's actually red and why:
Plan: the branch is already kept current with master. Once the (This supersedes my earlier note about the 🦉 via talyn.dev |
|
CI update — the branch is now rebased onto latest The current red Both report that they are sealed and carry 🦉 via talyn.dev |
CI status: current red checks are a fresh master-wide regression, not this PRThe earlier Root cause — #75140 (merged to master today, ~1h before this run). It broadened the
Proof it's master-wide, not this branch: unrelated PR #75701 ( Fix belongs on master — the two 🦉 via talyn.dev |
Fixed a second, pre-existing master breakage that blocked this product's testsAfter the master merge above turned Five source modules in Rather than block on it, I corrected the five imports to the canonical path (commit 🦉 via talyn.dev |
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Contained error-classification change in the warehouse-sources import pipeline (widens transient-error handling to include known DB connection blips, matching existing precedent elsewhere), plus a verified no-op import-path fix for a pre-existing breakage on 5 files. Author is on the owning team with strong recent familiarity, tests were added, and the behavior change only affects error-tracking noise, not sync correctness.
- Author wrote 100% of the modified lines and has 30 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 | ✓ | 43L, 6F substantive, 71L/8F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (71L, 8F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ b258ee3 · reviewed head 148078c |
|
Retaining stamphog approval — delta since last review classified as |
|
Retaining stamphog approval — delta since last review classified as |
…rite maintenance Pre-write maintenance (run_pre_write_defensive_compact) can hit an app-DB connection blip (DNS resolution failure, pooler-dropped connection) while resolving job.folder_path() on a pooled connection. Those surface as OperationalError/InterfaceError, are self-healing on the next sync's maintenance pass, and shouldn't be reported to error tracking as maintenance bugs. Add is_transient_maintenance_error, which broadens the existing object-store classification to also cover these DB connection blips, matching the OperationalError/InterfaceError classification already used by repartition_table.py's _is_transient_infra_error. The maintenance path uses the broadened check; is_transient_object_store_error is retained unchanged for the object-store-specific callers.
e346641 to
1a9f8ea
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Small, focused fix that broadens an existing transient-error classifier in a data-warehouse maintenance path to also treat DB connection blips as non-fatal, matching an established pattern elsewhere in the same codebase; author owns this code (owning team + STRONG familiarity, 100% of touched lines), added regression tests, and no reviewer concerns or holds are present.
- Author wrote 100% of the modified lines and has 28 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 | ✓ | 29L, 2F substantive, 55L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (55L, 3F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ e370063 · reviewed head 1a9f8ea |
Problem
Error tracking surfaced an
OperationalError: [Errno -2] Name or service not knownfromproducts/warehouse_sources/backend/temporal/data_imports/pipelines/common/extract.py, with the top in-app frame inExternalDataSchema.folder_path.The call chain:
run_pre_write_defensive_compact->DeltaTableHelper.run_maintenance->compact_if_fragmented->get_delta_table->_get_delta_table_uri, which resolves the Delta table's S3 URI viajob.folder_path()— a Django ORM lookup that traversesjob.schemaandschema.sourceFKs on a pooled app-DB connection. A DNS/pooler blip while opening that connection raised theOperationalError.This is a transient, self-healing infra blip, not a maintenance bug:
run_pre_write_defensive_compactalready catches every exception and skips a matching transient one with a quiet warning (the next sync's maintenance pass just retries from scratch). But the existing classifier (is_transient_object_store_error) only recognized S3/object-storeOSErrors from delta-rs, so this DB-connection error fell through tocapture_exceptioninstead — noise in error tracking for something that isn't a code defect.The same failure class (Postgres
OperationalError/InterfaceErrorfrom a transient connection blip) is already classified as transient infra elsewhere in this tree, inrepartition_table.py's_is_transient_infra_error.Changes
is_transient_maintenance_errorindelta_table_helper.py, which treatsdjango.db.OperationalError/InterfaceErroras transient on top of the existing S3/object-store substring matches. It delegates tois_transient_object_store_error, which stays unchanged so its other object-store-specific callers (get_delta_table,reset_table, CDC load) keep their narrower classification.extract.py'srun_pre_write_defensive_compactto use the broadenedis_transient_maintenance_errorclassifier.I found an open PR (#71934) that reduces how often this call path issues the underlying lazy DB query in the first place (via
select_related+ memoization). That's a complementary root-cause fix for the redundant query; it doesn't touch this classifier, so this change still closes the gap for connection blips on the remaining DB round-trip. I also found two open PRs (#69787, #69309) that suppress transient DNS/DB errors at the Temporal activity-interceptor level — those cover uncaught activity failures. This error never reaches that interceptor: it's caught and manually reported insiderun_pre_write_defensive_compactitself, so those PRs don't cover this code path.How did you test this code?
Extended
TestRunPreWriteDefensiveCompactintest_extract.pywithtest_logs_transient_db_connection_error_without_capturing, parameterized overOperationalError(DNS failure) andInterfaceError(dropped pooler connection) — assertscapture_exceptionis skipped and a warning is logged instead, mirroring the existing S3/object-store transient-error test right above it. This locks in the exact regression this PR fixes: without the classifier change, both cases would fall through tocapture_exceptionagain.Ran:
uv run pytest products/warehouse_sources/backend/temporal/data_imports/pipelines/common/test/test_extract.py -k TestRunPreWriteDefensiveCompact— 9 passeduv run mypy --cache-fine-grainedon the three changed files — cleanruff check --fix/ruff format— cleanAutomatic notifications
Docs update
N/A — internal error-classification change, no user-facing behavior change.
🤖 Agent context
Autonomy: Fully autonomous
Triaging a webhook-delivered error tracking issue. Used the PostHog MCP error-tracking tools (
query-error-tracking-issue,query-error-tracking-issue-events) to pull the stack trace, then an Explore subagent to readposthog/sync.py, thefolder_pathproperty chain,delta_table_helper.py, and existingNonRetryableErrors/transient-error precedent across the pipeline. Confirmed the DB round-trip and precedent (repartition_table.py's_is_transient_infra_error) directly before writing the fix. Invoked/writing-testsbefore adding the regression test. Checked for duplicate open PRs by exception type, message phrase, module path, and the maintainer's own open PRs — found related-but-non-overlapping work (see Changes section) rather than a duplicate.Created with PostHog Code