fix(declarative): ignore full-refresh sentinel in parent state seeding - #1079
Open
Ryan Waskewich (rwask) wants to merge 1 commit into
Open
fix(declarative): ignore full-refresh sentinel in parent state seeding#1079Ryan Waskewich (rwask) wants to merge 1 commit into
Ryan Waskewich (rwask) wants to merge 1 commit into
Conversation
Full refresh streams checkpoint {"__ab_no_cursor_state_message": true}. When a
later connector version converts such a stream to incremental with an
incremental_dependency parent, the single-value fallback in
_instantiate_parent_stream_state_manager re-keys the sentinel's boolean under
the parent's cursor field and cursor initialization crashes with
ValueError: No format in [...] matching True, before any records are read.
Filter the sentinel out of the child state before seeding parent state so the
converted stream starts its first incremental sync from empty state.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
👋 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@rwask/fix-full-refresh-sentinel-parent-state#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 rwask/fix-full-refresh-sentinel-parent-statePR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Ryan Waskewich (rwask)
marked this pull request as ready for review
July 27, 2026 15:08
Ryan Waskewich (rwask)
requested review from
Daryna Ishchenko (darynaishchenko) and
Anatolii Yatsuk (tolik0)
July 27, 2026 15:09
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughThe factory now removes the full-refresh cursor sentinel before initializing parent stream state during substream construction. A regression test verifies incremental dependency setup does not convert the sentinel into a parent cursor value. ChangesFull-refresh sentinel filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@unit_tests/sources/declarative/parsers/test_model_to_component_factory.py`:
- Around line 4067-4070: Update the assertion in the parent cursor state test to
require parent_cursor_state to equal an empty dictionary exactly, replacing the
permissive all(...) check while preserving the existing cursor-state setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: abf7a35e-c0c6-4694-85ef-c6ab788072fc
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.pyunit_tests/sources/declarative/parsers/test_model_to_component_factory.py
Comment on lines
+4067
to
+4070
| parent_cursor_state = stream.cursor._partition_router.parent_stream_configs[ | ||
| 0 | ||
| ].stream.cursor.state | ||
| assert all(value is not True for value in parent_cursor_state.values()) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the required empty parent state exactly.
Could we assert parent_cursor_state == {} instead? all(...) also passes for any non-boolean residual state, so it does not enforce the intended first incremental sync with empty state, wdyt?
Proposed test update
- assert all(value is not True for value in parent_cursor_state.values())
+ assert parent_cursor_state == {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| parent_cursor_state = stream.cursor._partition_router.parent_stream_configs[ | |
| 0 | |
| ].stream.cursor.state | |
| assert all(value is not True for value in parent_cursor_state.values()) | |
| parent_cursor_state = stream.cursor._partition_router.parent_stream_configs[ | |
| 0 | |
| ].stream.cursor.state | |
| assert parent_cursor_state == {} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@unit_tests/sources/declarative/parsers/test_model_to_component_factory.py`
around lines 4067 - 4070, Update the assertion in the parent cursor state test
to require parent_cursor_state to equal an empty dictionary exactly, replacing
the permissive all(...) check while preserving the existing cursor-state setup.
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
1 participant
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.
Summary
Converting a previously full-refresh stream to incremental with an
incremental_dependencyparent crashes cursor initialization whenever the connection has legacy state. Full refresh streams checkpoint{"__ab_no_cursor_state_message": true}; the single-value fallback in_instantiate_parent_stream_state_managerre-keys that boolean under the parent's cursor field (producing e.g.{"created_at": true}), andDatetimeStreamStateConverter.parse_valueraisesValueError: No format in [...] matching Truebefore any records are read. Every existing connection with such a stream selected fails on its next sync after upgrade — regardless of configured sync mode — until its state is reset.This filters the
NO_CURSOR_STATE_KEYsentinel out of the child state increate_parent_stream_config_with_substream_wrapperbefore it is used to seed parent stream state, so a converted stream starts its first incremental sync from empty state, matching howabstract_source.pyalready discards the sentinel.Found while validating airbytehq/airbyte#72784 (source-zoom), which had to revert its full-refresh → incremental conversion of existing streams because of this crash.
Note:
SubstreamPartitionRouter._migrate_child_state_to_parent_statehas the same boolean-passthrough flaw but currently has no callers, so it is left untouched here.Test plan
test_create_concurrent_cursor_from_perpartition_cursor_ignores_full_refresh_sentinel_state: on current main it reproduces the crash (ValueError: No format in ['%Y-%m-%dT%H:%M:%S.%f%z'] matching True); with the fix it passespoetry run pytest unit_tests/sources/declarative/parsers/ unit_tests/sources/declarative/partition_routers/— 237 passedruff format --checkandruff checkclean on changed filesairbyte/source-zoom:1.3.0-preview.6a84bb0(which declares incremental +incremental_dependencyon previously full-refresh meeting streams) exits 1 onreadwhen given{"__ab_no_cursor_state_message": true}stream state; the same read succeeds with this change applied to the installed CDKSummary by CodeRabbit
Bug Fixes
Tests