fix(data-imports): classify a concurrent reset losing a delta log commit file as transient - #75240
fix(data-imports): classify a concurrent reset losing a delta log commit file as transient#75240Gilbert09 wants to merge 6 commits into
Conversation
|
Merging to
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 |
|
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 |
🤖 CI report
|
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
tasks |
██████████████░░░░░░ 70.0% |
32,688 / 46,675 |
signals |
████████████████░░░░ 81.1% |
24,318 / 29,985 |
cdp |
████████████████░░░░ 82.1% |
3,285 / 3,999 |
data_modeling |
█████████████████░░░ 85.6% |
7,668 / 8,959 |
notebooks |
█████████████████░░░ 86.0% |
7,794 / 9,060 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
cohorts |
██████████████████░░ 87.8% |
6,181 / 7,040 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
exports |
██████████████████░░ 88.2% |
7,046 / 7,986 |
data_warehouse |
██████████████████░░ 88.4% |
12,192 / 13,798 |
engineering_analytics |
██████████████████░░ 89.3% |
6,529 / 7,309 |
conversations |
██████████████████░░ 89.3% |
17,600 / 19,701 |
dashboards |
██████████████████░░ 89.4% |
5,983 / 6,693 |
alerts |
██████████████████░░ 90.2% |
4,458 / 4,942 |
mcp_analytics |
██████████████████░░ 90.5% |
3,041 / 3,361 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.0% |
10,925 / 12,004 |
slack_app |
██████████████████░░ 91.1% |
9,632 / 10,576 |
stamphog |
██████████████████░░ 91.1% |
4,056 / 4,450 |
marketing_analytics |
██████████████████░░ 91.2% |
12,092 / 13,265 |
product_analytics |
███████████████████░ 92.5% |
5,849 / 6,321 |
early_access_features |
███████████████████░ 92.6% |
1,287 / 1,390 |
ai_observability |
███████████████████░ 92.8% |
15,618 / 16,821 |
surveys |
███████████████████░ 93.1% |
5,771 / 6,197 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
web_analytics |
███████████████████░ 93.3% |
14,911 / 15,976 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
workflows |
███████████████████░ 93.9% |
6,922 / 7,375 |
endpoints |
███████████████████░ 94.2% |
8,655 / 9,192 |
review_hog |
███████████████████░ 94.6% |
7,979 / 8,437 |
skills |
███████████████████░ 94.6% |
3,158 / 3,337 |
experiments |
███████████████████░ 95.4% |
25,938 / 27,177 |
logs |
███████████████████░ 95.5% |
10,435 / 10,928 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.4% |
17,474 / 18,130 |
replay_vision |
███████████████████░ 96.4% |
16,081 / 16,679 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 97.1% |
10,444 / 10,758 |
warehouse_sources |
███████████████████░ 97.3% |
358,420 / 368,487 |
data_catalog |
████████████████████ 97.8% |
2,589 / 2,648 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
…mit file as transient A vacuum racing a concurrent `reset_table` purge on the same table can lose a `_delta_log` commit file mid-read, surfacing as a `DeltaError`. Extend the existing racy-maintenance classifier to recognize this signature and apply it at the two call sites that were only checking the object-store classifier. Generated-By: PostHog Code Task-Id: 9ce798c6-8033-4db3-9459-872e787c9737
Adds a DeltaError case to the existing compaction/maintenance-failure-handling parameterized tests, closing the patch-coverage gap flagged by CI on the branches added in fb31377. Generated-By: PostHog Code Task-Id: 9ce798c6-8033-4db3-9459-872e787c9737
245cd29 to
2a7c706
Compare
…file Generated-By: PostHog Code Task-Id: 5c79a0ce-8b49-4dd5-865e-0d0e9cde711a
CI note:
|
…file Generated-By: PostHog Code Task-Id: 5c79a0ce-8b49-4dd5-865e-0d0e9cde711a
Update: root cause of the
|
…file Generated-By: PostHog Code Task-Id: c5c02917-c95d-4b03-96fe-c1228680996f
There was a problem hiding this comment.
Narrow, well-tested error-classification fix in already-swallowed best-effort Delta maintenance code (never re-raised, so sync correctness is unaffected); author owns this area with STRONG familiarity and added regression tests for both the positive and negative match cases.
- Author wrote 100% of the modified lines and has 37 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 | ✓ | 22L, 2F substantive, 52L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (52L, 4F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ c551d59 · reviewed head 1ac8c1e |
Problem
Error tracking issue surfaced a
DeltaErrorduring a Google Sheets full-refresh sync:The call path:
_post_run_operations->compact_table()->vacuum_table()-> delta-rs'stable.vacuum(), which needs to read a_delta_logcommit file to reconstruct the table's file history.Neither
vacuum()noroptimize.compact()ever delete a_delta_log/*.jsoncommit file themselves, so something else removed it mid-read. The prime suspect isreset_table(), which purges the whole table prefix (including_delta_log) at the start of a full-refresh sync — exactly the sync type on this event. If a still-running maintenance pass from an earlier, heartbeat-timed-out Temporal activity attempt (a "zombie", per the existing comments in this file) opened the table before a fresh attempt'sreset_table()purge landed, itsvacuum()call loses the commit file out from under it.This is the same category of race
TRANSIENT_DELTA_MAINTENANCE_ERRORS/is_transient_delta_maintenance_erroralready exists to classify (currently only foroptimize.compact's "Optimize selected-file scan failed" signature) — it's a caller losing a file to a concurrent maintenance/reset pass, not a corrupt table. The call sites that swallowcompact_table()/vacuum_table()failures (_post_run_operationsinpipeline.py,_run_delta_maintenanceincommon/load.py) already skip reporting foris_transient_object_store_error, but neither checkedis_transient_delta_maintenance_errorat all — so this DeltaError (and the existing "Optimize selected-file scan failed" one, at these specific call sites) fell through tocapture_exceptionas if it were a real bug.This is best-effort maintenance code in both cases: the exception is already caught and never re-raised, so the sync itself was never at risk. This only affects whether a self-healing race gets reported as a defect.
Changes
is_transient_delta_maintenance_errorindelta_table_helper.pyto also match aDeltaErrorwhose message contains both"File not found"and"_delta_log/"— the signature of a transaction-log commit file disappearing mid-read, distinct from a missing data file or a genuinely corrupt table (which still get captured).is_transient_delta_maintenance_errorcheck alongside the existingis_transient_object_store_errorcheck in_post_run_operations(pipeline.py) and_run_delta_maintenance(common/load.py), matching the patternrun_pre_write_defensive_compactalready uses incommon/extract.py.How did you test this code?
TestIsTransientDeltaMaintenanceErrorwith the missing-log-file signature (expectsTrue) and a "File not found" case outside_delta_log(expectsFalse, guarding against over-broadening the match) — this is the regression an unguarded revert of the new check would reintroduce.uv run pytest products/warehouse_sources/backend/temporal/data_imports/pipelines/core/test/test_delta_table_helper.py— 79 passed (5 pre-existing failures inTestGetDeltaTableUnrecoverableErrorsneed a running object-storage service not available in this sandbox; they fail identically on master).ruff check/ruff format --checkclean on all changed files.uv run mypy --cache-fine-grainedon the three changed source files — clean.Docs update
N/A — internal error-classification change, no user-facing or API surface change.
🤖 Agent context
Autonomy: Fully autonomous
Triaged from a live PostHog error-tracking issue (webhook-delivered) using Claude Code. Pulled the stack trace and
warehouse_sources_*event properties via the PostHog MCP tools, traced the call path intodelta_table_helper.py, and confirmed viais_deltatable/vacuum/optimize.compactsemantics that a missing log commit file mid-vacuum can only come from something else deleting it concurrently.Searched open PRs (by exception type, message phrase, module path, and the maintainer's own open PRs) before starting. Found three related-but-distinct open PRs touching the same file: #73454 (broadens the object-store classifier to
DeltaError, but only for S3-throttling substrings, not this signature), #74541 (fixes a different bug — a redundantget_delta_table()re-fetch racing a different table's cache eviction, raising a plainException, not thisDeltaError), and #74594 (disables delta-rs's automatic 30-day log-retention cleanup on writes/merges — a different trigger than a concurrentreset_tablepurge, and doesn't touch the classifier or the maintenance call sites this PR fixes). None overlap this fix.