Repository navigation
fix(low-code): document shared retrievers, lock session token refresh - #1201
Merged
Merged
Conversation
A stream reads all its partitions through one retriever, from several threads at once. The DeclarativePartitionFactory docstring still said the opposite and no extension point told custom component authors, so per-read state kept on a custom retriever or pagination strategy can silently drop records. - State the contract on DeclarativePartitionFactory, Retriever, PaginationStrategy and the CustomRetriever and CustomPaginationStrategy schema descriptions. - SessionTokenProvider broke it: concurrent calls each sent a login request, and the expiry was set before the token, so a call in between got "Failed to get session token, token is None". Refresh the cached token under a double-checked lock and set the token before its expiry. Without expiration_duration every call still logs in, unlocked. Fixes #1129
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@bazarnov/fix-shared-retriever-contract#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch bazarnov/fix-shared-retriever-contractPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Daryna Ishchenko (darynaishchenko)
approved these changes
Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
👉 TL;DR: Documents that a stream reads all its partitions in parallel through one shared set of components, so custom code must not keep per-read state on itself. It also fixes the built-in session login, which could log in several times at once, or fail with a missing token, when partitions started together.
Specifically, this states the contract on
DeclarativePartitionFactory,Retriever,PaginationStrategy,CustomRetrieverandCustomPaginationStrategy, and makesSessionTokenProviderrefresh its cached token under a double-checked lock and set the token before its expiry.Pairs with airbytehq/airbyte#88062, which updates the Custom Components docs page to the same contract:
Changes
DeclarativePartitionFactory.__init__docstring: replaced the stale text, which said every thread gets its own retriever, with what the code does. Every partition reads through the oneretriever, concurrently, which keeps one authenticator, HTTP session and async job repository per stream.RetrieverandPaginationStrategydocstrings: one instance serves every partition at once, so per-read state such as page tokens, counters or recursion depth belongs in local variables or in method arguments such aslast_page_token_value.CustomRetrieverandCustomPaginationStrategydescriptions indeclarative_component_schema.yaml: one sentence each with the same rule. The generated models do not include class descriptions, sodeclarative_component_schema.pyis unchanged.SessionTokenProvider: withexpiration_durationset, the refresh runs under a per-instance lock and re-checks the expiry once the lock is held, and_tokenis assigned before_next_expiration_time. Withoutexpiration_duration, every call still logs in, without the lock.test_token_provider.py: two tests. Two calls that find the token expired together send one login request and both get the new token. The token is already stored when its expiry is assigned.Review Spotlight
Reviewers with limited time, please review first:
token_provider.py#L53-L63: the double-checked lock, and the unlocked path without an expirytoken_provider.py#L78-L82: token before expiry, which the unlocked read inget_tokenrelies ontest_token_provider.py#L89-L132: the two concurrency tests and how they force the interleavingsretriever.py#L14-L21: the contract wording custom component authors will readRisks
SessionTokenAuthenticatorwithexpiration_duration. Concurrent partitions of a stream now share one login instead of each sending their own. A call can no longer fail withFailed to get session token, token is Nonewhile another call is logging in. That is 12 connectors, counted withgit grepover themanifest.yamlfiles on airbytemaster: 18 useSessionTokenAuthenticator(source-metabase uses the legacy class), and 12 of them setexpiration_duration.expiration_durationand keep logging in on every call, without the lock, as before.SessionTokenProvidercan no longer be deep-copied or pickled (TypeError: cannot pickle '_thread.lock' object). A factory-built one could before. Nothing copies one: the CDK only deep-copies manifests, configs, state, schemas and records. The 9deepcopy,copy.copyandpickle.dumpcalls in non-test source connector code on airbytemasterall copy data (git grep).RateLimitedMultipleTokenAuthenticatorin the same package already holds instance locks.datamodel-codegen0.26.3 output for the schema is byte-identical before and after.Test plan
Red/green. With the source files reverted to
origin/main(git checkout origin/main -- <source files>), both new tests fail in 100 out of 100 runs:test_session_token_provider_refreshes_once_for_concurrent_calls:assert 2 == 1(two login requests)test_session_token_provider_sets_token_before_expiry:assert [None] == ['my_token']With the fix, both pass in 200 out of 200 runs.
python -m pytest unit_tests/sources/declarative/auth unit_tests/sources/declarative/stream_slicers unit_tests/sources/declarative/retrievers unit_tests/sources/declarative/requesters/paginators unit_tests/sources/declarative/parsers -q: 866 passed.python -m pytest unit_tests/sources/declarative/test_declarative_component_schema_is_loadable.py unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py unit_tests/sources/declarative/test_concurrent_declarative_source.py unit_tests/sources/declarative/test_concurrent_declarative_source_config_overrides.py unit_tests/sources/declarative/interpolation/test_jinja.py unit_tests/sources/declarative/spec/test_spec.py -k "not test_read_with_concurrent_and_synchronous_streams" -q: 237 passed, 3 deselected. The deselected tests can crash on macOS because of the SQLite cache race tracked in Cached parent stream with more than one partition corrupts the shared sqlite request cache under concurrent reads (no such table: responses,file is not a database,no more rows available) #1150.ruff format,ruff checkandmypy --config-file mypy.inion the changed files: clean.prettier3.0.3 ondeclarative_component_schema.yaml: the same 5 findings as onmain, none on the edited lines.Design notes
Show/Hide Content
RLockthatAbstractOauth2Authenticatoruses: the guarded state belongs to the instance. A class-level lock would make logins of unrelated streams and connectors wait for each other, and a plain class-levelLockwould deadlock a login requester that authenticates with its ownSessionTokenAuthenticator.expiration_duration: nothing is cached, so every call needs its own login anyway, and a lock would only make those logins run one at a time.Barrieron the first two clock reads, and record the token at the moment the expiry is assigned. A call waiting on the lock cannot be observed, so a test that blocks the login on anEventwould need a timeout to tell a correct run from a broken one.Fixes #1129