Skip to content

fix(data-imports): recognize DeltaError as a transient object-store error - #73454

Merged
gantoine merged 2 commits into
masterfrom
posthog-code/fix-delta-error-transient-classification
Jul 30, 2026
Merged

fix(data-imports): recognize DeltaError as a transient object-store error#73454
gantoine merged 2 commits into
masterfrom
posthog-code/fix-delta-error-transient-classification

Conversation

@Gilbert09

Copy link
Copy Markdown
Member

Problem

Error tracking surfaced a DeltaError from run_pre_write_defensive_compact in products/warehouse_sources/backend/temporal/data_imports/pipelines/common/extract.py, during a Postgres incremental sync's pre-write maintenance pass (issue).

The call path: run_pre_write_defensive_compact -> DeltaTableHelper.run_maintenance -> vacuum_if_stale -> vacuum_table -> delta-rs's table.vacuum(), which needs to read a _delta_log manifest from S3. S3 responded with its SlowDown throttling error (503 Service Unavailable), and after exhausting delta-rs's internal retries the failure surfaced as a DeltaError.

This module already has a classifier for exactly this situation, is_transient_object_store_error, used by run_pre_write_defensive_compact to log a warning instead of capturing an error-tracking issue for a self-healing S3 blip (the docstring explicitly calls out that a maintenance failure must never block the actual sync). The problem: the classifier only recognized OSError, because that's the exception type DeltaTable.is_deltatable() raises for this same class of object-store error. Other table-level operations — vacuum(), optimize.compact() — wrap the identical underlying error text in deltalake.exceptions.DeltaError instead. Same transient blip, different Python exception type depending on which delta-rs entry point hit it, so the vacuum path's throttling error fell through to capture_exception and generated a noisy error-tracking issue for something that isn't a bug.

This is best-effort maintenance code, so the sync itself was never at risk — this only affects whether a self-healing throttling blip gets reported as a defect.

Changes

  • Broadened is_transient_object_store_error in delta_table_helper.py to also match deltalake.exceptions.DeltaError instances, not just OSError, against the same substring list.

How did you test this code?

Extended the existing parameterized test_logs_transient_object_store_error_without_capturing in test_extract.py with a DeltaError-wrapped case carrying the same S3 SlowDown error text — this is exactly the shape of the bug this PR fixes: without the classifier change, this case would fall through to capture_exception like the plain RuntimeError case in test_swallows_maintenance_failure above it.

Ran:

  • uv run pytest products/warehouse_sources/backend/temporal/data_imports/pipelines/common/test/test_extract.py -k TestRunPreWriteDefensiveCompact — 8 passed
  • uv run mypy --cache-fine-grained . (repo-wide, as CI runs it) — clean
  • ruff check --fix / ruff format — clean
  • hogli ci:preflight --fix — 0 failures

Automatic notifications

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

Docs update

N/A — internal error-classification change, no user-facing 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 the vacuum step of pre-write maintenance, not the Postgres source connector. Confirmed deltalake.exceptions.DeltaError isn't a subclass of OSError directly against the installed deltalake package before concluding this was the gap.

Checked for duplicate open PRs by exception type, message phrase, module path, and the maintainer's own open PR queue. Found two related-but-non-overlapping fixes in the same file: #73386 (classifies django.db connection errors as transient, a different exception source) and #73407 (adds S3 SlowDown text to the transient-error substring list, for the reset_table/_purge_s3_prefix path, still gated on isinstance(error, OSError)) — neither touches the OSError-vs-DeltaError gap this PR closes.

Invoked /writing-tests before extending the regression test.


Created with PostHog Code

@github-actions

Copy link
Copy Markdown
Contributor

Hey @Gilbert09! 👋

It looks like your git author email on this PR isn't your @posthog.com address (owerstom@gmail.com). Since you're on the PostHog team, it's worth pointing your local git author email at your @posthog.com address. Why it matters:

  • Consistent work identity in git history — internal tooling that attributes commits to team members keys off your @posthog.com address.
  • Keeps team contributions easy to tell apart from external community ones when scanning history.

You can fix it for this repo with:

git config user.email "you@posthog.com"

Or set it globally with git config --global user.email "you@posthog.com". No need to redo this PR — just a nudge for next time. 🙂

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team July 24, 2026 09:16
@Gilbert09 Gilbert09 added the stamphog Request AI approval (no full review) label Jul 24, 2026 — with PostHog
stamphog[bot]
stamphog Bot previously approved these changes Jul 24, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low-risk fix broadening a transient-error classifier to also match DeltaError alongside OSError, in best-effort maintenance logging code that doesn't gate the actual sync; change matches the PR description, is well-tested, and author is on the owning team with STRONG familiarity in these exact files.

  • 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 20L, 1F substantive, 39L/2F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1b-small (39L, 2F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ 2090a5a · reviewed head c0ecbd1

@Gilbert09
Gilbert09 force-pushed the posthog-code/fix-delta-error-transient-classification branch from c0ecbd1 to 4e2fe4f Compare July 28, 2026 15:21
@stamphog
stamphog Bot dismissed their stale review July 28, 2026 15:22

New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.

stamphog[bot]
stamphog Bot previously approved these changes Jul 28, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, well-tested broadening of an error classifier for a best-effort maintenance path; no risky territory touched, author owns the code with full familiarity.

  • Author wrote 100% of the modified lines and has 45 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 15L, 1F substantive, 34L/2F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1b-small (34L, 2F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ bf342ab · reviewed head 4e2fe4f

@hex-security-app

Copy link
Copy Markdown

Warning

Finding on products/streamlit_apps/backend/presentation/views.py:230 — could not attach an inline comment (line is not part of the diff), so reporting it here.

🟠 Require query access for session code uploads

A browser session bypasses APIScopePermission, so the extra query:read requirement here is enforced only for API-key/OAuth callers. AccessControlPermission consequently authorizes this action solely against streamlit_app; a user granted app write access but denied query access can upload and activate code, start it, and have its sandbox execute HogQL through the bridge. Passing that user to HogQL preserves table ACLs but does not perform the missing query-resource authorization, allowing a session user to bypass a query access-control restriction.

Prompt To Fix With AI
Enforce query-resource viewer access for session-authenticated callers on every action that can install, activate, or start executable Streamlit code (including create_version_from_source, upload_version, activate_version, start, and restart). Do not rely only on APIScopePermission, since it intentionally bypasses scope checks for sessions. Reuse the existing UserAccessControl/query authorization mechanism and add coverage for a session user with streamlit_app write permission but no query permission.

Severity: high | Confidence: 91% | React with 👍 if useful or 👎 if not

@trunk-io

trunk-io Bot commented Jul 28, 2026

Copy link
Copy Markdown

😎 Merged manually by @gantoine - details.

@Gilbert09
Gilbert09 force-pushed the posthog-code/fix-delta-error-transient-classification branch from a03b0fe to 033d0ad Compare July 29, 2026 14:58

Copy link
Copy Markdown
Member Author

Thanks — this finding is on products/streamlit_apps/backend/presentation/views.py, which this PR doesn't touch. It surfaced only because that file entered the branch's view via a routine master merge (the Streamlit apps feature), not from any change here. This PR is scoped to broadening the transient object-store error classifier in delta_table_helper.py, so the Streamlit access-control finding is out of scope for it and should be tracked against the code that introduced it.

🦉 via talyn.dev

@stamphog
stamphog Bot dismissed their stale review July 29, 2026 15:02

New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.

stamphog[bot]
stamphog Bot previously approved these changes Jul 29, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, well-tested fix broadening an error classifier to also recognize DeltaError for the same transient S3 throttling text; no risky territory (no schema/API/auth/billing/CI changes), diff matches description, and the only flagged comment concerns an unrelated file not touched by this PR.

  • Author wrote 100% of the modified lines and has 13 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 15L, 1F substantive, 34L/2F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1b-small (34L, 2F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ 7c4151e · reviewed head 033d0ad

…rror

`is_transient_object_store_error` only matched `OSError`, but delta-rs's table-level ops (`vacuum()`, `optimize.compact()`) wrap the same S3 error text in `deltalake.exceptions.DeltaError` instead, so an S3 SlowDown throttling blip during pre-write maintenance fell through to `capture_exception` instead of being logged as a warning.

Generated-By: PostHog Code
Task-Id: 36ba5ca6-7bdb-499b-805e-762d0600b738
@Gilbert09
Gilbert09 force-pushed the posthog-code/fix-delta-error-transient-classification branch from 033d0ad to aa220eb Compare July 30, 2026 12:42
@stamphog
stamphog Bot dismissed their stale review July 30, 2026 12:43

New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.

@talyn-app

talyn-app Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

/trunk merge

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, contained fix broadening an exception classifier for self-healing S3 throttling errors in best-effort maintenance code; author owns this area with strong familiarity, tests were extended to cover the exact bug, and the unrelated security finding raised in comments is on a different file that merely passed through via a master merge, not part of this diff.

  • Author wrote 100% of the modified lines and has 20 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 15L, 1F substantive, 35L/2F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1b-small (35L, 2F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ c033211 · reviewed head aa220eb

Copy link
Copy Markdown
Member Author

CI status: blocked by an unrelated master regression, not by this PR

This branch is rebased cleanly on top of latest master, approved, and mergeable. The only red check is one Backend CI shard:

Django tests – Core (persons-on-events off, events schema legacy) … (9/19)

The root failure is entirely unrelated to this PR (which only touches products/warehouse_sources/.../delta_table_helper.py and its test):

  • posthog/hogql/test/test_parser_cpp_json.py::TestParserCppJson::test_deeply_nested_input_does_not_stack_overflow fails with AssertionError: SyntaxError not raised : expr (the C++ parser's depth cap doesn't fire for the bracket-nesting case), which drops the shard's Postgres connection and cascades into ~381 OperationalError: the connection is closed failures in every subsequent test's setUp.
  • The Rust-backend equivalent (TestParserRustJson) passes.

This reproduces identically on master HEAD, so it isn't introduced here — it was introduced by #73958 (fix(hogql): cap recursion depth in the C++ parser), which merged ~1h ago: the Backend CI run for the commit immediately before it was green on this shard, and every master run since has failed on this same test. My own test shard (Django tests – Temporal …) is green, and a re-run of the failing shard on this branch failed identically, so it's deterministic rather than flaky.

Nothing to fix in this PR. Holding here until the parser regression is resolved on master; I'll re-merge master and re-verify CI as soon as it's green again.

🦉 via talyn.dev

…ssification

Generated-By: PostHog Code
Task-Id: b6b9ce1a-4805-483f-ba80-1cfca8ed089c

Copy link
Copy Markdown
Member Author

Update: the master regression is resolved — the C++ parser was bumped in #75473 (fix(hogql): bump hogql-parser to 1.3.83), and the previously-failing Core … (9/19) shard now passes on master. I re-merged latest master into this branch (server-side, two-parent merge — only this PR's two files appear in the diff), and CI is now fully green (141 pass, 0 fail). Branch is approved, mergeable, and up to date.

🦉 via talyn.dev

@talyn-app
talyn-app Bot enabled auto-merge (squash) July 30, 2026 15:36
@gantoine
gantoine disabled auto-merge July 30, 2026 16:46
@gantoine
gantoine merged commit bd91f59 into master Jul 30, 2026
233 checks passed
@gantoine
gantoine deleted the posthog-code/fix-delta-error-transient-classification branch July 30, 2026 16:47
@deployment-status-posthog

deployment-status-posthog Bot commented Jul 30, 2026

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-07-30 17:31 UTC Run
prod-us ✅ Deployed 2026-07-30 17:45 UTC Run
prod-eu ✅ Deployed 2026-07-30 17:49 UTC Run

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

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants