Repository navigation
fix(concurrent-cursor): don't advance parent state past unyielded child slices when parent generation fails - #1257
Conversation
…ld slices when parent generation fails
…ld slices when parent generation fails
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 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@devin/1793381933-cdk-parent-state-gate#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 devin/1793381933-cdk-parent-state-gatePR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)6 261 tests +38 6 239 ✅ +38 6m 36s ⏱️ + 1m 12s Results for commit 0d8c920. ± Comparison against base commit abb5be8. This pull request removes 3 and adds 41 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)6 264 tests +38 6 242 ✅ +38 12m 0s ⏱️ +23s Results for commit 0d8c920. ± Comparison against base commit abb5be8. This pull request removes 3 and adds 41 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Anatolii Yatsuk (tolik0)
left a comment
There was a problem hiding this comment.
Verified locally: the 5 tests listed as failing on main fail with main's cursor and pass at 0d8c920 (46/46 in the file); completed syncs are unchanged since _finished_generating_slices is set before the generation sentinel. A probe also confirms the gate covers GroupingPartitionRouter, where main lost up to group_size − 1 children per failure.
Summary
A declarative substream with
incremental_dependency: truecan persist a parent state that already covers a parent record whose child was never read. This happens when a parent slice request fails after the previous parent slice was closed. The skipped child is never read again, because the resumed sync starts the parent after that record.This PR changes one line:
ConcurrentPerPartitionCursor.ensure_at_least_one_state_emittednow refreshes_parent_statefrom the partition router only when the stream completed.Requested by ZaneHyattAB.
Why a record is lost
SubstreamPartitionRouter.stream_slices, the router callsparent_stream.cursor.close_partition(partition)on the last parent record of a slice, before it yields that record's child slice.ConcurrentPerPartitionCursor.stream_slicesconsumes the router throughiterate_with_last_flag_and_state, which callsnext()on the router before yielding the current child slice. When the next parent slice request raises, the previous slice's last child slice is held ascurrentand is never yielded.ensure_at_least_one_state_emittedsees every generated partition closed and ranself._parent_state = self._partition_router.get_stream_state()(line 298 onmain). That parent state includes the closed parent slice. It overwrote the low-water mark that_check_and_update_parent_statekeeps from closed child partitions, and the overwritten value was emitted.When the parent API returns newest first (as Stripe does), the lost record is the oldest record of the slice before the failing one. A resume with the emitted state starts at the newest record of that slice, so the lost child is never requested.
Fix
if all_partitions_closed: if self._synced_some_data and stream_completed: self._global_cursor = self._new_global_cursor self._lookback_window = self._timer.finish() - self._parent_state = self._partition_router.get_stream_state() + if stream_completed: + self._parent_state = self._partition_router.get_stream_state()stream_completedisall_partitions_closed and self._finished_generating_slices._finished_generating_slicesis set only after the router loop finishes, so syncs that complete are unchanged:ensure_at_least_one_state_emittedon a parent cursor only at its last slice, which is after that parent's generator is exhausted.When generation stops early, the emitted parent state is the one recorded for closed child partitions.
The same gate also stops one case of a progress-free state from being emitted: a global-cursor stream whose first parent request fails. On
mainthat case emits:{"use_global_cursor": true, "lookback_window": 0, "parent_state": {"posts": {"updated_at": "2024-01-01T00:00:00Z"}}}This PR also changes that case: the progress-free state is no longer emitted. For a plain substream on
mainthe state was harmless: an attempt resumed from it requests the same parent windows as a fresh attempt, starting at the start date (checked by a test run). It is the progress-free shape #1254 meant to suppress, though, because aStateDelegatingStreamtreats any non-empty state as "already synced once".Tests
All the new tests are in
unit_tests/sources/declarative/incremental/test_concurrent_perpartitioncursor.py. They read withConcurrentDeclarativeSourceand requests_mock. The mocked parent returns records newest first and filters them by thestart_time/end_timerequest parameters. The failure-path tests run a failing attempt, then resume from its last emitted state with the failure removed, and assert that the lost record's child is requested.maintest_given_failed_next_parent_request_when_resume_then_last_child_of_previous_parent_slice_is_read[global]test_given_failed_next_parent_request_when_resume_then_last_child_of_previous_parent_slice_is_read[per-partition]test_given_empty_parent_window_and_failed_next_parent_request_when_resume_then_child_is_readtest_given_no_state_and_failed_next_parent_request_when_resume_then_child_is_readtest_given_global_cursor_and_failed_first_parent_request_when_read_then_no_state_emittedtest_given_nested_parents_and_failed_middle_parent_request_when_resume_then_no_regressiontest_given_empty_parent_window_when_sync_succeeds_then_parent_state_is_newest_recordtest_given_successful_sync_when_read_then_parent_and_child_state_advance_to_newest_record[global]test_given_successful_sync_when_read_then_parent_and_child_state_advance_to_newest_record[per-partition]The nested test (posts → comments → votes) passes on
mainas well, so it doesn't reproduce the bug for nested parents. It's kept as a no-regression test. In it, post 1's day-2 comments request fails after its day-1 comments were returned newest first. The middle cursor stays at its input value because its partition for post 1 never finishes, so the resume re-reads day 1 and recovers the lost vote.The existing #1254 tests (
test_given_failed_*_when_ensure_state_emitted_*) pass unchanged.Local runs on this branch:
pytest unit_tests/sources/declarative/incremental/ unit_tests/sources/declarative/partition_routers/ unit_tests/sources/declarative/parsers/ unit_tests/sources/streams/concurrent/: 990 passed. After the latest test-only commit,incremental/passes 186 andpartition_routers/passes 82.ruff check .,ruff format --check .andmypy --config-file mypy.ini airbyte_cdk: clean.Exposure
incremental_dependency: trueappears 107 times across 39 connector manifests inairbytehq/airbyte. The counts come from a grep ofairbyte-integrations/connectors/**/*.yaml. The most affected are source-fastly and source-zendesk-support (10 each), source-track-pms (8), source-retailexpress-by-maropost (6), and source-dolibarr and source-stripe (5 each).Each of them can lose one parent record's children per sync in which a parent request fails. The loss is permanent when the parent cursor is a creation time. A parent cursored on an update time recovers the record on its next update.
Risk
The emitted parent state can now only lag behind the parent router, never lead it. The worst case is that a resumed sync re-reads parent records, and their children, from the last parent state that is fully processed.
Syncs that complete emit the same state as before.
Link to Devin session: https://app.devin.ai/sessions/74504a9af7b44c0ea9c97b52349f1b12
Open in Devin Desktop: https://app.devin.ai/desktop/session/74504a9af7b44c0ea9c97b52349f1b12?variant=devin