fix(data-imports): treat a racy optimize/vacuum scan failure as transient - #74635
Conversation
|
😎 Merged successfully - details. |
|
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 |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, well-tested reclassification of a known-transient delta-rs maintenance error to warning-level logging instead of error capture; no behavior change to sync correctness, retries, or data models. Author owns this code with strong familiarity and added targeted regression tests.
- Author wrote 100% of the modified lines and has 11 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 | ✓ | 28L, 2F substantive, 73L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (73L, 4F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 545937f · reviewed head 2af85ae |
|
/trunk merge |
…ient Pre-write delta maintenance (`run_pre_write_defensive_compact`) can hit a `DeltaError` when `optimize.compact`'s scan reads a file a concurrent maintenance pass has already vacuumed away. The scan fails before anything commits, so the table is left exactly as it was — just still fragmented — making this safe to skip and retry on the next pass, the same way an existing transient object-store error is already handled. Classify this specific delta-rs error signature the same way and log a warning instead of capturing it to error tracking. Generated-By: PostHog Code Task-Id: ee9ef592-a2cd-471f-a4a4-1520a5e26b1b
2af85ae to
383de70
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
|
/trunk merge |
|
Note 🤖 stamphog reviewed @hex-security-app[bot] still has a review in flight (👀) after 5 minutes — not approving over an unfinished review. The
Gate mechanics and policy version
|
Gilbert09
added
stamphog
There was a problem hiding this comment.
Small, well-tested reclassification of an already best-effort maintenance error path from captured-exception to warning-logged; the underlying try/except already prevented this from affecting the sync, so behavior for the actual data path is unchanged. Author is on the owning team with strong familiarity and full test coverage.
- 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 | ✓ | 27L, 2F substantive, 72L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (72L, 4F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ b21458f · reviewed head 383de70 |
fuziontech
left a comment
fuziontech
left a comment
There was a problem hiding this comment.
Automated review on behalf of @fuziontech.
Blocking: products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta_table_helper.py:88-94 classifies every DeltaError containing Optimize selected-file scan failed as transient, but that text identifies the optimize scan phase rather than the specific concurrent-vacuum 404 race. Corrupt Parquet, authorization failures, or persistently missing active files can carry the same wrapper and will now be omitted from error tracking by run_pre_write_defensive_compact. Please require the known missing-object/404 cause (or another structured indication of that race) and add a negative test with the same optimize prefix but a non-404 scan failure.
3 participants
Problem
An error tracking issue surfaced a
DeltaErrorduring a Postgres sync's append write:The stack trace traces through
run_pre_write_defensive_compact(common/extract.py) →DeltaTableHelper.run_maintenance→compact_if_fragmented→compact_table, landing in delta-rs'soptimize.compactcall.optimize.compactplans its rewrite against the table's file list at the start of a scan, then reads those files. If a concurrent maintenance pass on the same table (compact/vacuum can run from more than one call site across a sync, and a heartbeat-timed-out Temporal activity attempt can keep running as a "zombie" alongside its own retry — a race this package's README already documents for the equivalent repartition path) vacuums one of those files out from under the scan before it's read, delta-rs raises exactly this error.Since the scan fails before anything commits, the table is left exactly as it was — just still fragmented — so this is safe to skip and retry on the next maintenance pass.
run_pre_write_defensive_compactalready treats maintenance failures as best-effort (the sync isn't affected either way), but this specific error was still being captured to error tracking as if it were a bug, alongside an existing, narrower classifier (is_transient_object_store_error) for other known-transient blips.Changes
is_transient_delta_maintenance_errornext to the existingis_transient_object_store_errorindelta_table_helper.py, matching this specific delta-rs error signature.run_pre_write_defensive_compactnow logs a warning instead of capturing when this signature is hit, the same treatment already given to transient object-store errors.The same call chain also fans out into a couple of other pre-write/post-load maintenance call sites (
common/load.py,pipeline_v2/pipeline.py), where this same race is theoretically reachable too. I left those out of this PR: #74630 is already open and actively editing those exact same try/except blocks for a related but distinct S3-throttling signature, so extending them here risked a needless merge conflict. Once that one lands, extendingis_transient_delta_maintenance_errorto those call sites would be a natural, low-risk follow-up.No retry policy, timeout, or
NonRetryableErrorsclassification changed — this only reclassifies an already-non-fatal exception path.How did you test this code?
TestIsTransientDeltaMaintenanceErrortotest_delta_table_helper.py, parameterized over the racy scan signature, an unrelatedDeltaError, and a non-DeltaErrorexception carrying the same message — locks in that only the specific delta-rs signature matches.test_logs_transient_delta_maintenance_race_without_capturingtotest_extract.py'sTestRunPreWriteDefensiveCompact, mirroring the existing object-store-error case, assertingcapture_exceptionis not called and a warning is logged instead.test_delta_table_helper.py,test_extract.py,test_load.py, andpipeline_v2/test_pipeline.py: 122 passed, 4 pre-existing failures inTestGetDeltaTableUnrecoverableErrors(needs a live object-storage service unavailable in this sandbox; identical on unmodifiedmaster), several DB-backed tests erroring for the same sandbox reason.ruff check/ruff format --checkclean on all changed files.uv run mypy --cache-fine-grained .clean (17074 source files).Automatic notifications
Docs update
N/A — internal pipeline implementation detail, 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, exception sample, and
warehouse_sources_*properties via the PostHog MCP tools to confirm the exact call path and that it's a v2 (non-DLT) pipeline sync. Searched open PRs for duplicates: found #74630 touching the sameload.py/pipeline_v2/pipeline.pytry/except blocks for a different error signature (S3 throttling, not this optimize/vacuum race) — scoped this PR down to the file this issue's stack trace actually implicates (extract.py+delta_table_helper.py) to avoid duplicating or conflicting with that in-flight work. Also checked #74541 (a differentdelta_table_helper.pybug, redundant re-fetch inside compact/vacuum) for overlap — none, since it doesn't touch the classifier or call-site error handling this PR changes. Invoked/writing-testsbefore adding coverage.