fix(data-warehouse): retry UNAVAILABLE rpc status in temporalio source - #73361
Conversation
|
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 retry-logic fix (add gRPC UNAVAILABLE to a transient-retry set) in a data warehouse import helper; the other 56 files are purely mechanical import-reordering in auto-generated MCP tool files with no logic changes. No risky territory touched, cross-team authorship is only a routing signal here.
- Author wrote 3% of the modified lines and has 12 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 | ✓ | 13L, 1F substantive, 479L/58F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (479L, 58F, two-areas, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 4bf3017 · reviewed head 7537d5f |
|
🔀 Tried to auto-resolve conflicts with I won't retry until the branch or master moves. |
The Temporal.io warehouse import wraps its list/fetch calls in `_with_transient_rpc_retry` to ride out transient server blips, but the retryable status set only covered `RESOURCE_EXHAUSTED` and `DEADLINE_EXCEEDED` (plus a couple of message-matched cases). A DNS resolution blip on the client's connection to the cluster surfaces as gRPC status `UNAVAILABLE`, which fell outside both the status set and the message matchers, so it re-raised and failed the import activity instead of backing off. Adds `UNAVAILABLE` to the retryable status set alongside the existing statuses. Generated-By: PostHog Code Task-Id: 4f93ee01-af88-41c0-9adc-2f91b54e9a1a
7537d5f to
179b6a1
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Trivial, well-tested addition of a retryable gRPC status code by an author with strong familiarity on the owning team; no risky territory touched.
- Author wrote 100% of the modified lines and has 6 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 | ✓ | 13L, 1F substantive, 17L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1a-trivial (17L, 2F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 7ed0f2d · reviewed head 179b6a1 |
1 participant
Problem
The Temporal.io warehouse import wraps its
list_workflows/fetch_next_page/fetch_historycalls in_with_transient_rpc_retryto ride out transient server blips. The retryable set coversRESOURCE_EXHAUSTED,DEADLINE_EXCEEDED, and a couple of message-matched cases (h2 protocol error,Timeout expired), but not gRPC statusUNAVAILABLE.An error-tracking issue surfaced an
RPCErrorwith messagedns error(gRPC status code 14, i.e.UNAVAILABLE) — a transient connection-level failure resolving the cluster's frontend host. SinceUNAVAILABLEwasn't in the retryable set, the helper re-raised on the first attempt instead of backing off, failing the whole import activity (which rebuilds the client and restarts pagination) for what should have been a one-off blip.I checked for an existing fix first: #71792 attempted something similar (adding
UNAVAILABLEplus a message-fragment match), but it now conflicts with master (mergeable: false) since a separate change independently landed theh2 protocol error/Timeout expiredmessage-matching this PR's base already has. So it doesn't cleanly cover this case as-is; this PR addsUNAVAILABLEfresh against current master.Changes
Add
RPCStatusCode.UNAVAILABLEto_RETRYABLE_RPC_STATUSES— an unambiguously transient connection-level gRPC status, so it gets the same in-process backoff as the existing rate-limit and deadline cases. Persistent failures still re-raise so Temporal's activity-level retry applies.How did you test this code?
Extended the existing parametrized
TestTransientRPCRetrytests intest_temporalio.py:dns error/UNAVAILABLErow totest_rides_out_transient_error— locks in that this connection-level status now backs off and retries rather than failing immediately (the regression this PR fixes).test_persistent_transient_error_is_reraised— confirms a persistentUNAVAILABLEfailure still re-raises after exhausting attempts, same as the other retryable statuses.Ran
pytest products/warehouse_sources/backend/temporal/data_imports/sources/temporalio/test_temporalio.py::TestTransientRPCRetry(11 passed), plusruff check/ruff format --check.Automatic notifications
Docs update
No user-facing behavior or documented workflow changes.
🤖 Agent context
Autonomy: Fully autonomous
Authored by PostHog Code (Claude) triaging an error-tracking issue where a single
dns errorRPCError failed a Temporal.io warehouse import activity. Root cause:_RETRYABLE_RPC_STATUSESdidn't includeUNAVAILABLE(gRPC status 14), which is what a DNS resolution blip surfaces as.Before implementing, checked for a duplicate fix among open PRs (search on "temporalio", "RPCError", "dns error", plus the maintainer's own open PRs). Found #71792, which targets the same helper and a similar connection-level retry gap, but its base has since diverged (another change independently added message-based matching for
h2 protocol error/Timeout expired), leaving it in a conflicting state that doesn't apply cleanly. This PR makes the narrower, still-missing addition (UNAVAILABLEby status) against current master.Invoked
/writing-testsbefore extending test coverage — the added cases are parametrize rows on the existing retry tests, not new test functions.