fix(data-imports): classify exhausted delta commit-conflict retries as transient maintenance noise - #76402
Conversation
…s transient maintenance noise Delta maintenance (compact/vacuum) already retries a `CommitFailedError` by refreshing the table and re-running the operation, but sustained contention from a concurrent maintenance pass on the same table can exhaust that retry budget. When it does, the error was being captured as a fresh bug instead of treated like the other known concurrent-maintenance races that self-heal on the next scheduled pass. Extend the transient-maintenance classifier to also cover the three delta-rs conflict-checker race variants (concurrent append / delete-read / delete-delete), while still capturing a real MetadataChanged/ProtocolChanged commit failure. branch: posthog-code/fix-delta-maintenance-commit-conflict-transient Generated-By: PostHog Code Task-Id: 954359e0-a2e9-4dcc-948b-de23803dec4a
|
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 |
There was a problem hiding this comment.
Pull request overview
Adjusts delta maintenance error classification so exhausted commit-conflict retries (from concurrent writers during optimize.compact/maintenance) are treated as transient “maintenance noise” rather than captured as new defects. This keeps best-effort maintenance runs from generating error-tracking issues for self-healing concurrency races.
Changes:
- Extended
is_transient_delta_maintenance_errormatching to include the “Commit failed: a concurrent transaction …” commit-conflict message family. - Added new parameterized test cases covering commit-conflict variants (and a non-transient metadata-change commit failure).
- Extended
run_scheduled/maintenance handling tests to ensure these conflicts are warned-only (not captured) and never raised.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/test/test_maintenance.py | Adds a CommitFailedError case to the “never raises” behavior test to ensure exhausted commit-conflict retries are only warned. |
| products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/test/test_delta_errors.py | Adds parameterized cases for commit-conflict variants to the transient-classifier tests (and keeps a metadata-change case non-transient). |
| products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/errors.py | Extends the transient-maintenance substring list to include the concurrent-transaction commit-conflict signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…coverage The test now covers both the optimize-scan DeltaError signature and the CommitFailedError commit-conflict variants, so the old name (which only mentioned the scan case) made failures harder to interpret. Generated-By: PostHog Code Task-Id: 44141539-2628-4eee-9f52-3a60d2c54979
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, well-tested change to error-classification logic in best-effort delta maintenance code (exceptions here are already swallowed, never re-raised), authored by an owning-team member with a resolved Copilot review comment and no outstanding concerns.
- 👍 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 | ✓ | 17L, 1F substantive, 68L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (68L, 3F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 96e2437 · reviewed head 01e4a86 |
Problem
An error tracking issue surfaced a
CommitFailedErrorduring delta table maintenance:The call path:
run_scheduled->run_maintenance->compact_if_fragmented->_compact->execute_with_conflict_retry->optimize.compact()._compactalready routes throughexecute_with_conflict_retry, which retries aCommitFailedErrorby refreshing the table and re-running the operation (DELTA_MERGE_CONFLICT_RETRIEStimes). But sustained contention from another still-running maintenance pass on the same table (e.g. a second Temporal activity attempt still finishing a compact after the first attempt's heartbeat timed out) can exhaust that retry budget too. When it does, the error propagates out torun_scheduled's exception handler, which wasn't classifying it as one of the known self-healing concurrent-maintenance races — so it got captured as a fresh bug instead of logged as a warning.This is best-effort maintenance code: the exception was already caught by
run_scheduled's try/except and never re-raised, so no sync was ever at risk. This only affects whether a self-healing race gets reported as a defect.Changes
Extend
is_transient_delta_maintenance_error's classifier with theCommitFailedErrormessage shared by delta-rs's three concurrent-writer race variants (ConcurrentAppend,ConcurrentDeleteRead,ConcurrentDeleteDelete) — all of which start with "Commit failed: a concurrent transaction". Deliberately narrower than matchingCommitFailedErroroutright: aMetadataChanged/ProtocolChangedcommit failure isn't this same-pass race and should still be captured.How did you test this code?
TestIsTransientDeltaMaintenanceError,TestIsTransientMaintenanceError, andTestRunScheduled.test_never_raisescovering all three race message variants (classified transient) plus aMetadataChangedcommit failure (still captured).delta/test/suite (56 tests),ruff check/ruff format, andmypy --cache-fine-grainedrepo-wide — all clean.Docs update
N/A — internal maintenance-code fix, 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 samples, and
warehouse_sources_*event properties via the PostHog MCP tools to confirm the failing frame and reproduction context (a TemporalIO incremental sync), then read delta-rs's own conflict-checker source (vendored in the CI Rust toolchain cache) to confirm the exact set of concurrent-writer race message variants worth classifying as transient.Searched open PRs (by exception type, message phrase, module path, and the maintainer's own open PRs) before starting. Found related-but-distinct work on the same maintenance module — #76085 wraps
_vacuumin the same retry helper (a different frame losing the race with zero retries), #75240 classifies a differentDeltaErrorsignature (a missing_delta_logcommit file) as transient. Neither addresses this issue: aCommitFailedErrorthat already went throughexecute_with_conflict_retryand exhausted its budget.