Skip to content

fix(data-imports): disable automatic delta-rs log cleanup on writes/merges - #74594

Closed
Gilbert09 wants to merge 2 commits into
masterfrom
posthog-code/fix-delta-merge-log-cleanup-s3-error
Closed

fix(data-imports): disable automatic delta-rs log cleanup on writes/merges#74594
Gilbert09 wants to merge 2 commits into
masterfrom
posthog-code/fix-delta-merge-log-cleanup-s3-error

Conversation

@Gilbert09

Copy link
Copy Markdown
Member

Problem

Error tracking surfaced an OSError from the data-imports pipeline:

Generic S3 error
  ↳ Got invalid DeleteObjects response
  ↳ unknown variant `Code`, expected `Deleted` or `Error`

Error tracking issue

The stack trace traces through write_to_deltalake_do_merge → delta-rs's TableMerger.execute(), i.e. it fires inside an otherwise-successful MERGE commit, not during our own file handling.

Root cause

delta-rs runs a post-commit hook after every write/merge commit. By table default (delta.enableExpiredLogCleanup=true, 30-day logRetentionDuration), once a table's commit history crosses the retention window that hook batch-deletes expired _delta_log JSON files via the object_store crate's bulk DeleteObjects API — the same call class that TRANSIENT_OBJECT_STORE_ERRORS already guards against elsewhere in this file for is_deltatable()/_purge_s3_prefix.

Our storage backend can return a DeleteObjects response in a shape delta-rs's XML deserializer doesn't recognize, which fails the whole .execute() call even though the actual merge/write already committed. This only shows up once a table's log history passes the retention threshold, which matches this being a first-occurrence error on an established table rather than something hit on every sync.

Changes

We don't read _delta_log history ourselves and don't rely on delta-rs's automatic cleanup, so this disables it explicitly (post_commithook_properties=PostCommitHookProperties(cleanup_expired_logs=False)) on every write and merge call in delta_table_helper.py — both merge branches (partitioned/unpartitioned), the shared _write_deltalake helper, and the SCD2 close-merge + append write. Checkpoint creation is left untouched (harmless, unrelated to the bug).

How did you test this code?

Added 4 regression tests in TestPostCommitHookDisablesLogCleanup asserting post_commithook_properties=DISABLE_AUTO_LOG_CLEANUP is passed to each of the 5 delta-rs write/merge call sites (full-refresh write, partitioned merge, unpartitioned merge, SCD2 close-merge, SCD2 append write). Verified these tests fail with an ImportError when the fix is reverted, confirming they exercise the actual code path. Ran the full test_delta_table_helper.py suite (uv run pytest) — 70 passing, plus 4 pre-existing failures unrelated to this change (they require a local objectstorage/SeaweedFS service not available in this sandbox, and fail identically on master). Also ran ruff check, ruff format --check, and a repo-wide mypy --cache-fine-grained . — all clean.

Docs update

No user-facing behavior or API changed; no doc update needed.

🤖 Agent context

Autonomy: Fully autonomous

I (Claude, via PostHog Code) triaged this error-tracking issue end to end: pulled the stack trace and event properties from error tracking, read delta_table_helper.py and the pipeline_v3 batch consumer's retry classification (NON_RETRYABLE_ERROR_PATTERNS) to understand the call path, then inspected the installed deltalake 1.6.1 package source directly to find PostCommitHookProperties and confirm the post-commit-hook mechanism as the root cause, since no GitHub issue for this exact error string exists upstream.

Searched open PRs (by exception type, message phrase, module path, and the maintainer's own open PRs) before starting — found related-but-distinct PRs (#74388, #69306, #74541) touching the same file for different failure modes (credential blips, maintenance-op retries, table-reuse), none overlapping this post-commit-hook fix.

@trunk-io

trunk-io Bot commented Jul 29, 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

@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 29, 2026 10:36
@Gilbert09 Gilbert09 added the run-ci-backend Force ci-backend's full test matrices to run even on a draft PR label Jul 29, 2026 — with PostHog
@Gilbert09 Gilbert09 removed the run-ci-backend Force ci-backend's full test matrices to run even on a draft PR label Jul 29, 2026
@Gilbert09
Gilbert09 force-pushed the posthog-code/fix-delta-merge-log-cleanup-s3-error branch from 5cdca53 to dda00bd Compare July 29, 2026 11:42
@Gilbert09 Gilbert09 added the stamphog Request AI approval (no full review) label Jul 29, 2026 — with PostHog
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.

Contained bug fix disabling a delta-rs post-commit log-cleanup hook that was failing merges due to an S3 response-parsing mismatch; change is well-tested (4 new regression tests plus e2e assertions), author is on the owning team, and it doesn't touch schema, API contracts, auth, billing, or CI/deploy surfaces.

  • Author wrote 0% of the modified lines and has 10 merged PRs in these paths (familiarity MODERATE).
  • 👍 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 16L, 1F substantive, 107L/3F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (107L, 3F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ 545937f · reviewed head dda00bd

@Gilbert09
Gilbert09 force-pushed the posthog-code/fix-delta-merge-log-cleanup-s3-error branch from dda00bd to 5423013 Compare July 29, 2026 14:43
@stamphog
stamphog Bot dismissed their stale review July 29, 2026 14:45

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.

Contained fix disabling an unused delta-rs log-cleanup hook to avoid a spurious post-commit error; diff matches the description exactly, is covered by new regression tests plus updated e2e assertions, and the author is on the owning team for these files.

  • Author wrote 0% of the modified lines and has 13 merged PRs in these paths (familiarity MODERATE).
  • 👍 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, 106L/3F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (106L, 3F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ 30ad54b · reviewed head 5423013

@Gilbert09
Gilbert09 force-pushed the posthog-code/fix-delta-merge-log-cleanup-s3-error branch from 5423013 to f4267cb Compare July 30, 2026 10:30
@stamphog
stamphog Bot dismissed their stale review July 30, 2026 10:34

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

@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.

Contained, well-tested pipeline reliability fix by an owning-team author; disables a non-essential delta-rs cleanup hook that was causing spurious commit failures, with regression tests at every call site and no risky-territory surface (no schema/API/auth/billing/CI changes).

  • Author wrote 0% of the modified lines and has 18 merged PRs in these paths (familiarity MODERATE).
  • 👍 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 16L, 1F substantive, 107L/3F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (107L, 3F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ 0e33e42 · reviewed head f4267cb

@tests-posthog
tests-posthog Bot requested a review from a team as a code owner July 30, 2026 10:48
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

Playwright — all passed

All tests passed.

View test results →

⚠️ Backend snapshots — 13 updated (13 modified, 0 added, 0 deleted)

Query snapshots: Backend query snapshots updated

Changes: 13 snapshots (13 modified, 0 added, 0 deleted)

What this means:

  • Query snapshots have been automatically updated to match current output
  • These changes reflect modifications to database queries or schema

Next steps:

  • Review the query changes to ensure they're intentional
  • If unexpected, investigate what caused the query to change

Review snapshot changes →

@stamphog

stamphog Bot commented Jul 30, 2026

Copy link
Copy Markdown

Retaining stamphog approval — delta since last review classified as trivial_paths.

…erges

Every delta-rs write and merge commit runs a post-commit hook that, per table defaults, batch-deletes expired `_delta_log` JSON files once a commit's history crosses the retention window. That cleanup uses the same object_store bulk `DeleteObjects` call our storage backend can answer with a response shape delta-rs's XML parser doesn't recognize, failing the whole commit even though the write itself already succeeded.

We don't rely on this automatic cleanup, so disable it explicitly on every write and merge in `delta_table_helper.py`.

Co-Authored-By: PostHog Code

Generated-By: PostHog Code
Task-Id: 710c9b62-30ba-4ac7-b606-27fce571ea6b
…new post_commithook_properties kwarg

CI caught this: the previous commit added post_commithook_properties to every delta-rs write/merge call, but several e2e tests assert exact-equality on the kwargs dict passed to write_deltalake/merge, so they failed on the new key. Add it to each expected dict.

Generated-By: PostHog Code
Task-Id: 710c9b62-30ba-4ac7-b606-27fce571ea6b
@Gilbert09
Gilbert09 force-pushed the posthog-code/fix-delta-merge-log-cleanup-s3-error branch from e372eed to f97757c Compare July 30, 2026 11:15

Copy link
Copy Markdown
Member Author

Added the no-snapshot-update label. The snapshot-update bot pushed a test(backend): update query snapshots commit onto this branch that deleted ~7.7k lines across 13 unrelated .ambr files — including valid [new_events_schema] snapshots for tests like TestMaterializedColumnOptimization that still exist on master. That happens because this PR's matrix only ran the legacy-schema shards, so the new-events-schema variants' snapshots got pruned as "unused". None of that belongs in a data-imports fix, so I dropped the bot commit and switched the branch to CHECK mode to keep it from recurring. The PR diff is back to just the three intended files.

🦉 via talyn.dev

@Gilbert09 Gilbert09 closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-snapshot-update stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant