Repository navigation
fix(source-stripe): opt in to TCP keepalive and request timeouts - #88397
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
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
|
|
|
/ai-prove-fix
|
🧪 Fix Validation Evidence🟡 Outcome: No Regression — Fix Not ExercisedRegression tests (sandbox, all 19 streams) show no regression vs 6.0.22, but a healthy account never stalls, so they don't exercise the fix. In a local fault-injection run with a stalled upstream, the control (6.0.22) hung with no output for 21m45s until we killed it. The target raised ➡️ Next Steps
📋 Connector & PR DetailsConnector: 📝 Evidence PlanProving Criteria: On the target image, a stalled READ request fails with a timeout and is retried; on control it blocks indefinitely. Healthy reads show no regression across all streams. ✅ Pre-flight Checks
🔍 Detailed Evidence LogRegression test (run):
Fault injection (TLS-terminating proxy for
Not exercised: TCP keepalive on a dead peer or NAT drop, the 30 s connect timeout on its own, and any live/customer connection. |
Airbyte Support Bot (airbyte-support-bot)
added
the
hyd-prove
|
↪️ Triggering Reason: Current-HEAD prove-fix verdict is recorded (NOT_EXERCISED) and no AI review exists for this HEAD.
|
Reviewing PR for connector safety and quality.
|
🛡️ AI PR Review Report🟡 Review Action: NO ACTION (INCONCLUSIVE)
|
| Gate | Status | Enforced? | Details |
|---|---|---|---|
| PR Hygiene | PASS | Yes | Detailed description, a 6.0.23 changelog row in docs/integrations/sources/stripe.md, and no unresolved human review comments |
| Code Hygiene | PASS | WARNING | manifest.yaml changed and the new test unit_tests/test_requester_settings.py was added |
| Test Coverage | PASS | Yes | Behavioral change (title contains "fix"); the new tests test_every_http_requester_opts_in and test_built_http_clients_use_settings assert on the settings |
| Code Security | PASS | Yes | No security path patterns; no auth/credential keywords in +/- lines |
| Per-Record Performance | PASS | WARNING | Only requester session settings changed; no per-record code paths |
| Breaking Dependencies | WARNING | WARNING | SDM/CDK 7.33.0 → 7.35.0 (includes 7.34.0 cursor, partition and pagination fixes) |
| Backwards Compatibility | PASS | Yes (enforced) | No spec, schema, stream, PK or cursor changes; patch bump only |
| Forwards Compatibility | PASS | Warning (elevates Risk Level) | No state, cursor, partition, pagination or transformation changes in connector code; rollback is safe |
| Behavioral Changes | FAIL | Warning (elevates Risk Level) | Timeout keywords in the manifest.yaml diff |
| Out-of-Scope Changes | PASS | Skip | All files are under source-stripe/ or docs/ |
| CI Checks | PASS | Yes | Connector CI, Test and Lint source-stripe all green (230 tests, 0 failed) |
| Live / E2E Tests | UNKNOWN | Yes | /ai-prove-fix NOT_EXERCISED for this HEAD |
PR Hygiene evidence
- PR Body Length (raw): ~4,900 chars
- PR Body Length (after stripping): ~4,900 chars (no HTML comments)
- PR Body Length (visible content): well above 50 chars
- PR Body Preview:
## What <table><tr><td> **:point_right: TL;DR:** Stops Stripe syncs from hanging forever when a netwo... - Changelog:
| 6.0.23 | 2026-10-09 | [88397](...) | Opt in to TCP keepalive and request timeouts ... - Peer feedback: only
github-actions[bot]markdownlint inline comments, which are ignored as bot comments; theDocs / MarkDownLintcheck passed.
Test Coverage evidence
- Behavioral Change Detected: Yes
- Indicators Found: title keyword
fix - Test Files Modified:
unit_tests/test_requester_settings.py(new),unit_tests/poetry.lock - New Test Content Found: Yes
- Test Content Evidence:
def test_every_http_requester_opts_in() -> None:
Code Security: Changed lines in manifest.yaml are only use_tcp_keepalive, connect_timeout_in_seconds and read_timeout_in_seconds. The authenticator line is unchanged context. Changed lines in metadata.yaml are baseImage and dockerImageTag; connectorBuildOptions and allowedHosts are unchanged context.
Backwards Compatibility: No spec file or spec section was modified, so there is no required array change. No schemas/ or stream definitions changed. The prove-fix regression showed no spec or catalog diffs.
Behavioral Changes: Keywords matched in +lines are timeout (connect_timeout_in_seconds: 30, read_timeout_in_seconds: 80). The effect is that a request stalled for more than 80 s between bytes now raises ReadTimeout and is retried within the existing error-handler window, instead of hanging. A slow but alive Stripe endpoint that goes more than 80 s without sending bytes would now error. Given Stripe's paginated list reads, and that 80 s matches Stripe's own SDK default, this is unlikely, but it is a semantics change across all streams.
Breaking Dependencies: The CDK release notes for v7.34.0 include ConcurrentCursor end-window and clamping changes, SubstreamPartitionRouter skipping null parent keys, and OffsetIncrement/PageIncrement re-read fixes. Stripe relies on substreams and concurrent cursors, so these can subtly change the record set. Mitigation: the regression run showed RECORD 1787 → 1787 and STATE 265 → 265, with equal per-stream counts. The unit_tests/poetry.lock bumps (pydantic 2.11.7 → 2.13.5, pydantic-core 2.33.2 → 2.46.5) are test-only.
Live / E2E Tests: Validation is required (bug fix plus a requester change). The prove-fix marker prove_fix_result: NOT_EXERCISED; head_sha: 63a566a2… maps to UNKNOWN.
📚 Evidence Consulted
Evidence
- Changed files: 5 files (
manifest.yaml,metadata.yaml,unit_tests/poetry.lock,unit_tests/test_requester_settings.py,docs/integrations/sources/stripe.md) - CI checks: Connector CI Checks Summary ✅, Test source-stripe Connector ✅, Lint source-stripe Connector ✅, Format Check ✅, Docs / MarkDownLint ✅, Docs / Vale ✅, Build and Verify Artifacts ✅. Excluded from this gate: source-stripe Pre-Release Checks ✅, Progressive Rollout Gate ✅, Validate PR Title ✅.
- PR labels:
connectors/source/stripe,hyd-prove,hyd-review(no validation override labels) - PR description: present
/ai-prove-fix: comment — NOT_EXERCISED for the current HEAD- Existing bot reviews: none (only
github-actions[bot]COMMENTED)
❓ How to Respond
Resolving a Failing Gate
Each non-PASS gate above lists concrete remediation. Apply it, push, then re-run /ai-review. The bot re-evaluates from the diff, CI check-runs, and /ai-prove-fix results.
- Live / E2E Tests (UNKNOWN): Re-run
/ai-prove-fixonce a connection exhibits the stall pattern, or validate via/ai-canary-prereleaseor a pinned prerelease on the affected connection from https://github.com/airbytehq/oncall/issues/12977, as the PR's Rollout section suggests. Progressive rollout is already enabled.
Written explanations do not change a gate verdict. If you believe a gate is a false positive or it cannot be remediated as written, request review from a human maintainer and explain the situation to them in a PR comment. A human reviewer can approve or merge over a bot FAIL. Never edit the PR description to argue with the bot; the description is how reviewers learn what the PR does.
|
🔴 Escalated for human review per Hands-Free AI Triage Project daily sweep. Reason:
|
master now carries 6.0.23 (auto deps bump, SDM 7.35.0) and 7.0.0 (#87765), so this PR becomes 7.0.1 with only the opt-in requester settings, the coverage test and the unit-test lock bump. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZaneHyattAB
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
4 participants
What
👉 TL;DR: Stops Stripe syncs from hanging forever when a network connection silently dies or gets dropped mid-request: the connector now detects dead connections and gives up on silent responses, then retries.
Specifically,
base_requestersetsuse_tcp_keepalive: true,connect_timeout_in_seconds: 30andread_timeout_in_seconds: 80, which are new insource-declarative-manifest7.35.0(already the base image onmaster) (airbytehq/airbyte-python-cdk#1168).Today the Stripe requester has no timeout and no keepalive. If a connection dies or is dropped by a NAT while the connector waits for a response, the sync can block indefinitely: it stays alive, emits nothing and never exits. Context: https://github.com/airbytehq/oncall/issues/12977
Requested by Zane Hyatt (ZaneHyattAB).
How
manifest.yaml: the three settings go ondefinitions.base_requester. Every requester in the manifest resolves tobase_requester:eventsretriever used by the incremental side of everyStateDelegatingStream;files/file_links.So this one edit covers every stream. The top-level
versionis unchanged: the CDK validates against its runtime schema, and the manifest migrations are version-wildcard.metadata.yaml:dockerImageTaggoes7.0.0→7.0.1. The base imagesource-declarative-manifest:7.35.0is already onmaster(auto deps bump deps(source-stripe): 🐙 update dependencies [2026-10-13] #88809), so this PR no longer changes it.unit_tests/poetry.lock:airbyte-cdkgoes7.29.0→7.35.0, plus the bumps it requires (pydantic,pydantic-core,typing-inspection), so the tests exercise the new fields.New
unit_tests/test_requester_settings.pyasserts two things:HttpRequesterin the resolved manifest carries the three settings;(30, 80)timeout and the keepalive adapter. This is checked stateless and on the events path.Changelog row in
docs/integrations/sources/stripe.md.Why these values
stripe-pythontimeout=80). It bounds the gap between bytes, not the total request time. Within the manifest error handler's 600 s retry window it leaves room for about 7 attempts; 300 s would allow about 2.limit=100) onapi.stripe.com/v1, with smallexpand[]lists.files/file_linkslist metadata only.Review guide
airbyte-integrations/connectors/source-stripe/manifest.yamlairbyte-integrations/connectors/source-stripe/metadata.yamlairbyte-integrations/connectors/source-stripe/unit_tests/test_requester_settings.pydocs/integrations/sources/stripe.md,unit_tests/poetry.lockUser Impact
A request that previously could hang forever on a dead or NAT-dropped connection now fails and is retried. Healthy syncs are unaffected.
Rollout
Progressive rollout is enabled for Stripe. Per the reviewer's suggestion, do a
/prereleaseand a pinned run on a real connection before wide adoption.Not fixed by this PR
bank_accountsandcustomer_balance_transactions. These are per-customer substreams over very large customer lists, so syncs including them stay slow; they just can't hang silently anymore.Local validation
airbyte-cdk image buildsucceeded andspecworks; the image containsairbyte-cdk7.35.0.checkSUCCEEDED and areadofcustomers,invoicesandinvoice_line_itemsreturned 1544 records with no errors.Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/56ec2115e89a41e99baa6ae024af93b2
Open in Devin Desktop: https://app.devin.ai/desktop/session/56ec2115e89a41e99baa6ae024af93b2?variant=devin
Important
Autopilot Progressive Rollout Enabled
Autopilot progressive rollouts are enabled for one or more connector(s) modified in this PR. Check the box below if you need to bypass normal rollout safety processes and release to all users immediately upon merge:
Note: