fix(source-zendesk-support): skip refused tickets on side_conversations instead of failing the sync - #84353
Conversation
…ns instead of failing the sync Access to side conversations is granted per ticket, not per stream: the feature requires the Collaboration add-on and can be restricted per brand and per group, and the tickets incremental export also returns deleted tickets. Zendesk therefore refuses individual tickets while the rest of the stream reads fine. The stream's error handler tried to tolerate that with an `error_message_contains: "You do not have access"` IGNORE filter, but the collaboration-api service answers these denials with a 403 carrying an empty `text/html` body. `HttpResponseFilter._response_contains_error_message` parses the body as JSON, so the filter never matched and the request fell through to the `[403, 404] -> FAIL / config_error` filter below it, killing the whole sync on the first refused ticket. Replace both filters with status-code-keyed IGNOREs for 403 and 404, matching the pattern the stateful `ticket_metrics` path has used on the same class of per-ticket endpoint since before 5.2.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Note 📝 PR Converted to Draft More info...Thank you for creating this PR. As a policy to protect our engineers' time, Airbyte requires all PRs to be created first in draft status. Your PR has been automatically converted to draft status in respect for this policy. As soon as your PR is ready for formal review, you can proceed to convert the PR to "ready for review" status by clicking the "Ready for review" button at the bottom of the PR page. To skip draft status in future PRs, please include |
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
Detected
|
|
Deploy preview for airbyte-docs ready!
Deployed with vercel-action |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regression test results — fix verifiedComparison run (PR build vs published 5.5.1), warm read, all 41 streams:
The red workflow status is purely because the control (published 5.5.1) fails, which is exactly the bug this PR fixes. Control (5.5.1) — dies on the first refused parent ticket: 74 distinct parent tickets returned 403 with an empty body, so the Target (this PR) — skips those tickets and completes: That message appears 74 times (one per refused ticket), the stream finishes, and the sync exits cleanly (exit code 0). Other streams: only Note on the connection used: the affected customer connection (EU data residency) cannot be regression-tested — credential retrieval is blocked for EU connections, so that run aborted at SPEC (https://github.com/airbytehq/airbyte-ops-mcp/actions/runs/31714876562). The run above uses another Tier-2 connection with The 404 and 422 branches were not exercised — no such responses occurred on this connection. |
|
/publish-connectors-prerelease
|
|
↪️ Triggering Reason: PR is ready for review, regression results against published 5.5.1 are posted, and no AI review has run on this branch yet. |
Reviewing PR for connector safety and quality.
|
🛡️ AI PR Review Report💬 Review Action: Comment (no approval)🟡 Risk Level: 2 / 5 (Low-Moderate)Small, well-tested manifest-only error-handling change on a single substream, backed by a live target-vs-control regression run. Not approved only because CI is not green: the two failing connector tests are
📋 PR Details
🔍 Gate Evaluation Details1. PR Hygiene — ✅ PASS 2. Code Hygiene — 3. Test Coverage — ✅ PASS 4. Code Security — ✅ PASS 5. Per-Record Performance — ✅ PASS 6. Breaking Dependencies — ✅ PASS 7. Backwards Compatibility — ✅ PASS 8. Forwards Compatibility — 9. Behavioral Changes — 10. Out-of-Scope Changes — ✅ PASS 11. CI Checks — ❓ UNKNOWN 12. Live / E2E Tests — ✅ PASS 📚 Evidence Consulted
🛠️ How to RespondThe only non-
Re-request review with Review generated by Devin · gates: 8 PASS / 3 WARN / 1 UNKNOWN / 0 FAIL |
|
🙋 Escalated per Hands-Free AI Triage Project — the AI review at |
3 participants
What
Resolves https://github.com/airbytehq/oncall/issues/13285.
A sync fails outright, every run, when Zendesk refuses side conversations for a single parent ticket:
followed by the sync-killing:
Access to side conversations is granted per ticket, not per stream. The feature requires the Collaboration add-on, access can be restricted per brand and per group, and the
ticketsincremental export also returns deleted tickets. So Zendesk refuses individual tickets while the rest of the stream reads normally — a refusal must skip that ticket, not fail the sync.The stream's error handler tried to tolerate exactly this, but keyed on the response body:
Denials from Zendesk's
collaboration-apiarrive as a403with an emptytext/htmlbody.HttpResponseFilter._response_contains_error_messageparses the body withJsonErrorMessageParser, which yields nothing for a non-JSON body, so the IGNORE filter cannot match and the request falls through to theFAILfilter below it. The same emptiness is why the user-facing message ends inError message: None—{{ response.get('error') }}interpolates against_safe_response_json, which returns{}.Why this surfaced in 5.5.0 and not earlier
side_conversationswas added in 5.3.0, whileticketswas reading the Export Search Results endpoint. That endpoint is served from Zendesk's search index, which excludes deleted tickets — so those tickets never became substream partitions. #81640 revertedticketsto the Incremental Ticket Export endpoint, which returns them.side_conversationshas therefore never run against this parent in any released version; the pairing is new as of 5.5.0, not restored.Why it cannot self-heal
side_conversationsis a substream withincremental_dependency: true, so the parent cursor is only checkpointed once the substream finishes. Because it never finishes,parent_statenever advances — an affected connection restarts the same parent walk and dies on the same ticket every run, indefinitely, re-walking months of parent tickets each time (which is also where the rate-limit pressure on these jobs comes from). Observed on a live connection:side_conversations.parent_statefrozen at the state-migration floor whileticket_metrics.parent_stateon the same connection and same parent had advanced to the current day.How
Replaced the body-keyed IGNORE and the
[403, 404] FAILpair with status-code-keyed IGNOREs:This is not a new pattern for this connector. The stateful
ticket_metricspath requests the same class of per-ticket endpoint (GET /tickets/{ticket_id}/metrics) and has used status-code-keyed403/404IGNOREs since before 5.2.0 — its404message even reads "Not found. Ticket was deleted." That stream stays healthy on connections whereside_conversationsdeadlocks, which makes it the control case for this change.Both new messages state the condition and the likely cause without the misleading remediation prose, and neither interpolates a value that cannot resolve. Note the error-message context here is limited to
config,response,headersand$parameters—stream_partitionis not available, so the messages deliberately do not attempt to name the ticket.Relationship to #83708
That open draft makes the shared requester handler stricter (403/404 → FAIL, no body-substring IGNORE) so stream-level permission denials stop producing silent zero-record successes. This PR moves
side_conversationsin the opposite direction, and both are correct: the shared handler serves stream-level endpoints where a denial means no data at all, whileside_conversationsis per-partition where a denial means one ticket. The two changes touch adjacent lines and will need a trivial conflict resolution; they should not be reconciled into a single policy.Declarative-First Evaluation
Declarative only. No Python component was added or modified —
DefaultErrorHandlerandHttpResponseFilteralready express the needed behavior, and the defect was which condition the filters keyed on.Breaking Change Evaluation
Not breaking. No change to schema, spec, primary key, cursor, stream set, or state format; no migration needed.
It does change an outcome: syncs that failed with a
config_errorwill now succeed, with the refused tickets' side conversations absent and one INFO log per skipped ticket. Stated plainly, the trade-off is that a complete loss of side-conversations access would now read as an empty stream rather than a failure. That is accepted here because the refusal is partition-scoped and indistinguishable, at the HTTP layer, from the per-ticket case — the same tradeticket_metricshas always made. Version5.5.2.Test Coverage
unit_tests/mock_server/test_side_conversations.py:test_given_403_with_empty_html_body_when_read_then_ignore_and_continue— the real failure shape; fails on master, passes here.test_given_404_when_read_then_ignore_and_continue— deleted ticket; fails on master.test_given_one_ticket_denied_when_read_then_other_tickets_still_sync— two parent tickets, one refused with an empty-body 403, asserts the readable one still yields its record and no errors are emitted; fails on master.test_given_403_when_read_then_ignore_and_continue— 403 carrying Zendesk's JSON error envelope. Passes on master too (the old body filter did catch this variant), kept to lock in that both body shapes are handled.Added
ErrorResponseBuilder.with_empty_html_body()so the empty non-JSON body can be reproduced at all; every existing builder emitted a JSON envelope, which is precisely why this path had no coverage.Full connector suite: 217 passed, 2 failed. Both failures are in
mock_server/test_tickets.pyon thetickets_searchstream (test_given_lookback_window_...,test_when_read_tickets_search_then_partitions_produce_correct_query_params) and reproduce identically on unmodified master — pre-existing, unrelated to this change.Review guide
manifest.yaml— the three filters, and whether404 → IGNOREis the right call for a deleted parent ticket.ticket_metricsalready makes it.AGENTS.md§5 — documents the body-vs-status trap and theparent_statedeadlock for whoever adds the next per-parent substream.Follow-up not in this PR
Seven other substreams request one URL per parent record and inherit the shared handler with no override, so a single refused or deleted parent record fails those syncs the same way:
article_attachments,article_comments,article_votes,article_comment_votes,post_comments,post_votes,post_comment_votes. #83708 making the shared handler stricter will make that latent failure more likely, not less. Worth a dedicated PR rather than widening this one.Also noted: the stateful
ticket_metrics403 message contains a literal{self.name}, which is not interpolated by the declarative framework and renders verbatim. Left alone here to avoid conflicting with #83708, which already touches that line.Can this PR be safely reverted and rolled back?
Important
Active progressive rollout warning for source-zendesk-support.