fix(data-imports): keep transient S3 credential blips out of error tracking - #74388
Conversation
|
😎 Merged manually by talyn-app[bot] - 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.
The "credential" title flag is about a transient S3 IMDS/STS credential-provider blip in error classification, not an authentication/authorization change — no auth surface touched. This is a contained error-tracking/observability fix in warehouse-sources code, owned and authored by a STRONG-familiarity team member with new regression tests covering the exact gap.
- Author wrote 100% of the modified lines and has 52 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 | ✓ | 30L, 2F substantive, 88L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (88L, 4F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 1ebb50f · reviewed head 043483c |
043483c to
1934daa
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Small, well-tested fix broadening transient object-store error classification in the data-imports pipeline; "auth" title flag is a false positive (S3 IMDS/STS credential blips, not app auth), author is on the owning team with strong familiarity, and no risky territory is touched.
- Author wrote 100% of the modified lines and has 27 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 | ✓ | 30L, 2F substantive, 88L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (88L, 4F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 30ad54b · reviewed head 1934daa |
|
/trunk merge |
2 similar comments
|
/trunk merge |
|
/trunk merge |
3124812 to
01fba76
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
|
/trunk merge |
There was a problem hiding this comment.
Narrow error-classification fix that reclassifies a known transient S3/IMDS credential blip so it isn't reported as a defect; diff matches the description, has targeted regression tests, and the 'auth' title flag is incidental (a credential-provider exception type, not an authentication/authorization flow).
- Author wrote 100% of the modified lines and has 31 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, 83L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (83L, 4F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 8bba179 · reviewed head 01fba76 |
|
/trunk merge |
1 similar comment
|
/trunk merge |
01fba76 to
ce7545c
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
|
/trunk merge |
There was a problem hiding this comment.
The "auth" flag is incidental — the change only reclassifies transient S3/IMDS credential-provider errors so Temporal doesn't report them to error tracking, it doesn't touch any authentication or authorization logic. Small, well-tested, single-area fix by the owning team's most familiar author, no unresolved concerns.
- Author wrote 100% of the modified lines and has 33 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, 83L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (83L, 4F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ cdc53cb · reviewed head ce7545c |
🤖 CI report
|
|
Retaining stamphog approval — delta since last review classified as |
|
/trunk merge |
2b3cd58 to
8c27f0b
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
|
/trunk merge |
There was a problem hiding this comment.
Contained error-classification fix in the data-imports pipeline: broadens transient-object-store-error detection and re-raises it as NonReportableError so Temporal still retries but error tracking isn't paged. "Credential" in the title refers to an S3/IMDS credential-provider blip, not authentication logic — diff confirms no auth surface touched. Author owns the code with STRONG familiarity, tests cover both new branches, no outstanding review concerns.
- 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 | ✓ | 27L, 2F substantive, 83L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (83L, 4F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ ec35a5a · reviewed head 8c27f0b |
|
Rebased onto latest Heads-up on CI: the only red backend check is Repo checks ( The fix for those two products (re-narrowing contract-check inputs / relocating facade re-exports) is a product-structure decision owned by those teams, so I've deliberately kept it out of this PR. Once 🦉 via talyn.dev |
CI status — remaining red checks are pre-existing / infra, not from this PRThis PR is up to date with The checks still showing red are all external to this PR:
This PR's own regression tests pass locally:
I've reported the two CI breakages (product-lint on 🦉 via talyn.dev |
|
Update: the repo-wide CI on the merged head surfaced a second, separate pre-existing 🦉 via talyn.dev |
Fixed the warehouse-sources product-test failure (stale
|
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Contained error-classification fix in the warehouse-sources data-import pipeline (recognizing a bare botocore NoCredentialsError as a transient object-store blip, and wiring the existing classifier into the generic error handler so Temporal still retries but error tracking isn't paged); the "auth" title flag is incidental — it's about AWS credential-provider resolution against PostHog's own bucket, not user authentication. Author owns the code (STRONG familiarity, on the owning team), the accompanying import-path fixes are unrelated pre-existing breakage explained in the thread, and the change ships with targeted regression tests.
- Author wrote 100% of the modified lines and has 42 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 | ✓ | 41L, 6F substantive, 99L/9F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (99L, 9F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 8f2331e · reviewed head 1eee8c2 |
…acking
Error tracking surfaced an `OSError: Operation not supported... the credential provider was not enabled... no providers in chain provided credentials`, raised during a full-refresh table reset in the shared data-imports pipeline. Digging into the stack trace, the run also hit a chained `botocore.exceptions.NoCredentialsError` ("Unable to locate credentials") from `reset_table`'s `_purge_s3_prefix` S3 delete call.
Both are the same class of transient blip already documented in `TRANSIENT_OBJECT_STORE_ERRORS`/`is_transient_object_store_error` (IMDS/STS credential-provider hiccups against our own instance-role-authenticated data-warehouse bucket, not a customer credential problem). Two gaps let this specific occurrence through as error-tracking noise:
1. `is_transient_object_store_error` only recognized `OSError`. `_purge_s3_prefix`'s s3fs/aiobotocore call can raise a bare, unwrapped `NoCredentialsError` for the identical blip - a different exception type depending on which client library hit it.
2. The classifier was only consulted in one call site (`handle_corrupted_delta_log`'s reset-failure handler). Everywhere else a transient object-store error escapes the pipeline run, it falls through `_handle_import_error`'s final branch, which logs `aexception` and lets the raw exception re-raise - and the activity interceptor reports any re-raised exception to error tracking regardless of log level, since it only skips reporting for `NonReportableError` (and a couple of other known-benign types).
Generated-By: PostHog Code
Task-Id: f9004b94-2306-496f-9536-fbf8a56f02e6
7a919f4 to
c48c56b
Compare
Rebased onto
|
…rror-tracking Generated-By: PostHog Code Task-Id: ff2bd760-da74-4e0d-889d-d034d17af9b9
1 participant
Problem
Error tracking surfaced an
OSError: Operation not supported... the credential provider was not enabled... no providers in chain provided credentialsfrom the shared data-imports pipeline, during a full-refresh table reset. The chain also included abotocore.exceptions.NoCredentialsError("Unable to locate credentials") fromreset_table's_purge_s3_prefixS3 delete call in the same run.Both are the transient object-store blip class already documented by
TRANSIENT_OBJECT_STORE_ERRORS/is_transient_object_store_errorindelta_table_helper.py- an IMDS/STS credential-provider hiccup against our own instance-role-authenticated data-warehouse bucket, not a customer credential problem, and self-healing on retry. Two gaps let this specific occurrence through as error-tracking noise:is_transient_object_store_erroronly recognizedOSError._purge_s3_prefix's s3fs/aiobotocore call can raise a bare, unwrappedbotocore.exceptions.NoCredentialsErrorfor the identical blip - a different exception type depending on which client library hit it (delta-rs's Rustobject_storecrate vs. aiobotocore's own credential resolution).handle_corrupted_delta_log's reset-failure handler). Everywhere else a transient object-store error escapes the pipeline run, it falls through_handle_import_error's final branch, which logs ataexceptionand re-raises the raw exception. The activity interceptor (posthog/temporal/common/posthog_client.py) reports any exception that escapes an activity to error tracking regardless of log level - it only skips reporting forNonReportableErrorand a couple of other known-benign marker types.This is already retryable today (nothing here is in any source's
NonRetryableErrors, and Temporal's default activity retry policy applies), so the sync itself was never at risk - this only affects whether a self-healing blip gets reported as a defect.Changes
is_transient_object_store_errorindelta_table_helper.pyto also recognize a barebotocore.exceptions.NoCredentialsErrorby type (its message is a fixed generic string, so there's no substring to match, but hitting our own bucket always means the same transient resolution hiccup)._handle_import_errorinimport_data_sync.pynow checksis_transient_object_store_errorbefore the generic fallback, alongside the existingRESTClientRetryableErrorhandling. Since log level alone doesn't suppress interceptor reporting, it re-raises asNonReportableError(chained viafrom error) instead of the bare exception, so Temporal still retries the activity as usual but the interceptor no longer reports it.How did you test this code?
TestIsTransientObjectStoreErrorintest_delta_table_helper.pycovering the existingOSErrorsubstring match, the new bareNoCredentialsErrorcase, and two negative cases (unrelatedOSError, unrelated exception type) - guards the exact gap that let this occurrence through.test_transient_object_store_error_reraised_as_non_reportableintest_import_data_sync.py, asserting_handle_import_errorlogs a warning (notaexception) and raisesNonReportableErrorchained to the original error - catches a regression back to re-raising the bare exception, which the activity interceptor would still report.Ran:
uv run pytest products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline/test/test_delta_table_helper.py -k TestIsTransientObjectStoreErrorand.../workflow_activities/tests/test_import_data_sync.py- all passuv run mypy --cache-fine-grained .(repo-wide, as CI runs it) - cleanruff check --fix/ruff format --check- cleanhogli ci:preflight --fix- 0 failuresAutomatic notifications
Docs update
N/A - internal error-classification/reporting change, no user-facing or API 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 chained stack trace. Traced the call path from the Temporal activity interceptor down throughpipeline.run()intoreset_table/_purge_s3_prefix, and separately confirmed (by inspecting the installeddeltalakepackage's compiled extension) that the arrow-formatted OSError text originates in delta-rs's Rustobject_store/aws-configcrate, not in PostHog code or s3fs/botocore.Checked for duplicate open PRs by exception type, message phrase, and module path, and scanned the maintainer's own open PR queue. Found three related-but-non-overlapping PRs in the same files: #73454 (broadens the classifier to
deltalake.exceptions.DeltaError, a different exception source), #73458 (adds a retry loop inside_purge_s3_prefixitself, gated on the same classifier), and #74378 (removes a spuriousget_delta_table()re-fetch in pipeline cleanup that was chaining an unrelated secondOSErroronto other failures). None of them touch theNoCredentialsErrorclassification gap or the generic_handle_import_errorreporting gap this PR closes.Invoked
/writing-testsbefore adding the regression tests.