Invalidate cached ContinuationIterable when its continuation fails#39384
Invalidate cached ContinuationIterable when its continuation fails#39384addenergyx wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the Python SDK's state caching mechanism where permanently invalid continuation tokens could lead to infinite retry storms. By ensuring that failed state reads invalidate the corresponding cache entry, the system can now recover from transient or permanent runner-side token revocations instead of repeatedly attempting to use stale data. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to invalidate cached iterables in the SDK worker when a continuation fails, preventing permanently invalid continuation tokens from remaining in the cache. A unit test has been added to verify this behavior. The review feedback suggests wrapping the cache invalidation callback in a try-except block to ensure that any failure during invalidation does not mask the primary exception raised during iteration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if self.on_failure is not None: | ||
| self.on_failure() |
There was a problem hiding this comment.
If self.on_failure() raises an exception, it will mask the original exception raised by self.continue_iterator_fn(). To prevent masking the primary failure and ensure it is correctly propagated, we should wrap the on_failure() call in a try...except block and log any invalidation failures.
| if self.on_failure is not None: | |
| self.on_failure() | |
| if self.on_failure is not None: | |
| try: | |
| self.on_failure() | |
| except Exception: | |
| _LOGGER.warning('Failed to invalidate cache on continuation failure.', exc_info=True) |
|
Closing for now, will reopen after further review. |
57c518f to
f2362fa
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39384 +/- ##
=========================================
Coverage 58.04% 58.05%
Complexity 13051 13051
=========================================
Files 2518 2518
Lines 263804 263822 +18
Branches 10764 10764
=========================================
+ Hits 153138 153160 +22
+ Misses 104911 104907 -4
Partials 5755 5755
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f2362fa to
7cb7243
Compare
GlobalCachingStateHandler caches partially materialized bag state (ContinuationIterable) across bundles under the user state cache token. The iterable's lazy remainder holds the continuation token minted by the read that created it. If that token becomes permanently invalid (for example the runner revoked it together with the work item that created it), the cached iterable fails on every subsequent read of that state and the cache never recovers: each retry is served the same poisoned object and replays the dead token instead of re-fetching. On Dataflow this manifests as an unbounded 'work token no longer valid' retry storm that stalls the stage. Drop the cache entry when iterating the continuation fails so the next read loads fresh state. The invalidation is identity-guarded via a new StateCache.invalidate_if_value so a stale iterable failing again later cannot evict a newer value cached under the same key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7cb7243 to
604ff1b
Compare
Fixes #39383
GlobalCachingStateHandler.blocking_getcaches partially materialized bag state as aContinuationIterable, keyed by the user-state cache token. The iterable's lazy tail holds the continuation token from the read that created it. If the runner permanently invalidates that token (Dataflow revokes them together with the work item that created them), the cached object fails on every later read of that state and the cache never recovers: each retry is served the same object and replays the dead token instead of re-fetching. See #39383 for the production symptoms.This PR invalidates the cache entry when iterating the continuation fails, so the next read loads fresh state. A transient failure costs one extra reload; a permanent one self-heals instead of retrying indefinitely. The invalidation goes through a new identity-guarded
StateCache.invalidate_if_value, so a stale iterable failing again later cannot evict a newer value cached under the same key, and a failure inside the invalidation itself is logged rather than allowed to mask the original exception. Backward compatible:ContinuationIterable.on_failuredefaults toNone.Validation via new deterministic tests (no Dataflow required; the fake state handler uses epoch-scoped token revocation so revoked tokens fail while fresh reads succeed, matching real runner semantics):
test_failed_continuation_invalidates_cached_iterable,test_failed_continuation_invalidates_after_partial_yield,test_stale_failed_iterable_does_not_evict_replacement,test_invalidate_if_value,test_invalidate_if_value_ignores_in_flight_load, andtest_invalidation_failure_does_not_mask_continuation_error.RuntimeError: continuation token no longer validfrom the dead token.sdk_worker_test.py(including all pre-existing tests) andstatecache_test.pypass, 40 tests total.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more information on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.
🤖 Generated with Claude Code