Replace workflows forceStale flag with a forceRefresh param#3541
Merged
vegaro merged 6 commits intoJun 8, 2026
Conversation
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vegaro
changed the title
forceStale flag with a forceRefresh param
vegaro
force-pushed
the
cesar/wfl-280-replace-forceworkflowslistcachestale-with-a
branch
from
cbe7276 to
44c2a5c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3541 +/- ##
==========================================
+ Coverage 80.18% 80.19% +0.01%
==========================================
Files 371 371
Lines 15233 15239 +6
Branches 2111 2112 +1
==========================================
+ Hits 12214 12221 +7
+ Misses 2169 2168 -1
Partials 850 850 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vegaro
force-pushed
the
cesar/workflow-detail-swr
branch
from
8e422d6 to
8509e19
Compare
vegaro
force-pushed
the
cesar/wfl-280-replace-forceworkflowslistcachestale-with-a
branch
from
44c2a5c to
6f9ba69
Compare
Member
|
I opened #3555 against this branch with a focused failing test for the It demonstrates that when a forced workflows-list refresh fails while an old list cache is still fresh, the next non-forced Leaving it as a draft/repro PR so you can decide how to handle the behavior. |
vegaro
pushed a commit
that referenced
this pull request
Jun 8, 2026
## Summary Adds a regression test that demonstrates the failed `forceRefresh` retry path in PR #3541. The test covers this sequence: 1. A workflows list fetch succeeds and caches `default -> wf_old`. 2. A later `forceRefresh = true` fetch runs while that old list is still fresh, but the backend request fails. 3. The next normal `getWorkflowsList(..., forceRefresh = false)` should retry and update the map to `wf_new`. On the current implementation from #3541, step 3 is skipped because the old workflows-list timestamp remains fresh after the failed forced refresh. ## Validation This is a repro PR, so the added test is expected to fail until the implementation changes. ```bash rtk ./gradlew :purchases:testDefaultsBc8DebugUnitTest --tests "com.revenuecat.purchases.common.workflows.WorkflowManagerTest.getWorkflowsList retries after failed forceRefresh even when old list cache was fresh" ``` Failure observed on PR #3541 head: ```text Verification failed ... 2 matching calls found, but needs at least 3 and at most 3 calls ``` Before adding this repro, `WorkflowManagerTest` and `WorkflowsCacheTest` passed on PR #3541 head in the focused run. <details> <summary>AI session context</summary> # AI Context ## Metadata - PR: Not captured - Branch: `facundo/pr3541-force-refresh-retry-test` - Author / human owner: `facumenzella` - Agent(s): Codex - Session source: current conversation - Generated: 2026-06-08 - Context document version: 1 ## Goal Review PR #3541, verify whether a suspected workflows-list `forceRefresh` retry regression is real, then open a repro PR against PR #3541 so the original author can decide how to handle it. ## Initial Prompt Review `https://github.com/RevenueCat/purchases-android/pull/3541`. ## Important Follow-up Prompts - User asked to write a test and prove the suspected issue is broken. - User then asked to open a PR against PR #3541 so `vegaro` can decide. ## Agent Contribution - Reviewed PR #3541 diff against its stacked base branch. - Identified a behavioral difference: replacing `forceWorkflowsListCacheStale()` with call-scoped `forceRefresh` no longer clears the old workflows-list timestamp before the forced fetch. - Added a focused failing unit test that proves a failed forced refresh is not retried by the next non-forced list call while the old cache timestamp is still fresh. - Ran the focused Gradle test command and captured the failure. - Created and pushed branch `facundo/pr3541-force-refresh-retry-test`. ## Human Decisions - Human accepted the review finding as worth proving with a test. - Human decided to open a PR against PR #3541 rather than directly changing the implementation. ## Key Implementation Decisions - Decision: Add only a regression test, not a production fix. - Rationale: The purpose is to provide reproducible evidence and let PR #3541's author decide the implementation. - Rejected: Directly patching `WorkflowManager` in this PR. ## Files / Symbols Touched - `purchases/src/test/java/com/revenuecat/purchases/common/workflows/WorkflowManagerTest.kt` - Why: Add the smallest unit-level repro for the retry behavior. - Symbols: `WorkflowManagerTest`, `getWorkflowsList retries after failed forceRefresh even when old list cache was fresh` - Review relevance: Confirms whether a failed forced refresh should leave the list retryable even when the previous list timestamp was fresh. ## Dependencies / Config / Migrations - None captured. ## Validation - Commands run: - `rtk ./gradlew :purchases:testDefaultsBc8DebugUnitTest --tests "com.revenuecat.purchases.common.workflows.WorkflowManagerTest.getWorkflowsList retries after failed forceRefresh even when old list cache was fresh"`: failed as expected with only two `Backend.getWorkflows` calls observed where the test expected three. - `rtk ./gradlew :purchases:testDefaultsBc8DebugUnitTest --tests "com.revenuecat.purchases.common.offerings.OfferingsManagerTest" --tests "com.revenuecat.purchases.common.workflows.WorkflowManagerTest" --tests "com.revenuecat.purchases.common.workflows.WorkflowsCacheTest"`: `WorkflowManagerTest` and `WorkflowsCacheTest` passed before adding the repro; `OfferingsManagerTest` failed at class initialization because Robolectric selected Android SDK 36 with Java 17. - Manual verification: - Inspected the failing test output and confirmed the skipped third backend call. - CI: - Not captured. ## Validation Gaps - The repro PR intentionally contains a failing test against PR #3541's current implementation. - `OfferingsManagerTest` was not validated locally due to Java/Robolectric SDK mismatch. ## Review Focus - Should a failed forced workflows-list refresh keep the list stale/retryable for the next call? - If yes, decide whether to restore timestamp-clearing behavior, track failed forced refresh state, or use another retry signal. ## Risks / Reviewer Notes - Risk: The test encodes a desired retry behavior rather than current behavior. - Evidence: It fails on PR #3541 head with only two backend list calls. - Mitigation: Treat this PR as a repro and decision aid, not a final implementation. ## Non-goals / Out of Scope - Production fix for the retry behavior. - Broader refactor of workflows list caching. ## Omitted Context - Raw transcript, unrelated exploration, sensitive details, repetitive attempts, and chain-of-thought-style content were omitted. </details>
Member
Author
|
@facumenzella thanks for that. Fixed in 5c62525 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4b68fd3. Configure here.
vegaro
commented
Jun 8, 2026
| // Clear detail caches after a successful fetch so the prefetch loop below is a | ||
| // guaranteed cache miss and always populates fresh data. Cleared here rather than | ||
| // before the network call so a failed fetch leaves in-memory details intact. | ||
| if (forceRefresh) workflowsCache.clearWorkflowDetailCaches() |
Member
Author
There was a problem hiding this comment.
This is a fix for a regression I noticed in main. Force refresh wasn't working
…orceRefresh param forceWorkflowsListCacheStale() was a side-channel flag: it cleared the list cache timestamp, and the next getWorkflowsList consulted it. That coupled two calls (force then fetch), and any completing in-flight fetch re-stamped the cache fresh, quietly swallowing the force. Replace it with a forceRefresh parameter on getWorkflowsList, OR'd into the staleness check, so the "fetch fresh" intent travels with the call. Behavior is preserved: a fresh offerings network response passes forceRefresh = true; a disk-cache replay passes false. The in-flight dedup still joins an existing batch (a joined batch returns a live backend response, so "force fresh" is satisfied) — now documented in the getWorkflowsList KDoc instead of emergent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
## Summary Adds a regression test that demonstrates the failed `forceRefresh` retry path in PR #3541. The test covers this sequence: 1. A workflows list fetch succeeds and caches `default -> wf_old`. 2. A later `forceRefresh = true` fetch runs while that old list is still fresh, but the backend request fails. 3. The next normal `getWorkflowsList(..., forceRefresh = false)` should retry and update the map to `wf_new`. On the current implementation from #3541, step 3 is skipped because the old workflows-list timestamp remains fresh after the failed forced refresh. ## Validation This is a repro PR, so the added test is expected to fail until the implementation changes. ```bash rtk ./gradlew :purchases:testDefaultsBc8DebugUnitTest --tests "com.revenuecat.purchases.common.workflows.WorkflowManagerTest.getWorkflowsList retries after failed forceRefresh even when old list cache was fresh" ``` Failure observed on PR #3541 head: ```text Verification failed ... 2 matching calls found, but needs at least 3 and at most 3 calls ``` Before adding this repro, `WorkflowManagerTest` and `WorkflowsCacheTest` passed on PR #3541 head in the focused run. <details> <summary>AI session context</summary> # AI Context ## Metadata - PR: Not captured - Branch: `facundo/pr3541-force-refresh-retry-test` - Author / human owner: `facumenzella` - Agent(s): Codex - Session source: current conversation - Generated: 2026-06-08 - Context document version: 1 ## Goal Review PR #3541, verify whether a suspected workflows-list `forceRefresh` retry regression is real, then open a repro PR against PR #3541 so the original author can decide how to handle it. ## Initial Prompt Review `https://github.com/RevenueCat/purchases-android/pull/3541`. ## Important Follow-up Prompts - User asked to write a test and prove the suspected issue is broken. - User then asked to open a PR against PR #3541 so `vegaro` can decide. ## Agent Contribution - Reviewed PR #3541 diff against its stacked base branch. - Identified a behavioral difference: replacing `forceWorkflowsListCacheStale()` with call-scoped `forceRefresh` no longer clears the old workflows-list timestamp before the forced fetch. - Added a focused failing unit test that proves a failed forced refresh is not retried by the next non-forced list call while the old cache timestamp is still fresh. - Ran the focused Gradle test command and captured the failure. - Created and pushed branch `facundo/pr3541-force-refresh-retry-test`. ## Human Decisions - Human accepted the review finding as worth proving with a test. - Human decided to open a PR against PR #3541 rather than directly changing the implementation. ## Key Implementation Decisions - Decision: Add only a regression test, not a production fix. - Rationale: The purpose is to provide reproducible evidence and let PR #3541's author decide the implementation. - Rejected: Directly patching `WorkflowManager` in this PR. ## Files / Symbols Touched - `purchases/src/test/java/com/revenuecat/purchases/common/workflows/WorkflowManagerTest.kt` - Why: Add the smallest unit-level repro for the retry behavior. - Symbols: `WorkflowManagerTest`, `getWorkflowsList retries after failed forceRefresh even when old list cache was fresh` - Review relevance: Confirms whether a failed forced refresh should leave the list retryable even when the previous list timestamp was fresh. ## Dependencies / Config / Migrations - None captured. ## Validation - Commands run: - `rtk ./gradlew :purchases:testDefaultsBc8DebugUnitTest --tests "com.revenuecat.purchases.common.workflows.WorkflowManagerTest.getWorkflowsList retries after failed forceRefresh even when old list cache was fresh"`: failed as expected with only two `Backend.getWorkflows` calls observed where the test expected three. - `rtk ./gradlew :purchases:testDefaultsBc8DebugUnitTest --tests "com.revenuecat.purchases.common.offerings.OfferingsManagerTest" --tests "com.revenuecat.purchases.common.workflows.WorkflowManagerTest" --tests "com.revenuecat.purchases.common.workflows.WorkflowsCacheTest"`: `WorkflowManagerTest` and `WorkflowsCacheTest` passed before adding the repro; `OfferingsManagerTest` failed at class initialization because Robolectric selected Android SDK 36 with Java 17. - Manual verification: - Inspected the failing test output and confirmed the skipped third backend call. - CI: - Not captured. ## Validation Gaps - The repro PR intentionally contains a failing test against PR #3541's current implementation. - `OfferingsManagerTest` was not validated locally due to Java/Robolectric SDK mismatch. ## Review Focus - Should a failed forced workflows-list refresh keep the list stale/retryable for the next call? - If yes, decide whether to restore timestamp-clearing behavior, track failed forced refresh state, or use another retry signal. ## Risks / Reviewer Notes - Risk: The test encodes a desired retry behavior rather than current behavior. - Evidence: It fails on PR #3541 head with only two backend list calls. - Mitigation: Treat this PR as a repro and decision aid, not a final implementation. ## Non-goals / Out of Scope - Production fix for the retry behavior. - Broader refactor of workflows list caching. ## Omitted Context - Raw transcript, unrelated exploration, sensitive details, repetitive attempts, and chain-of-thought-style content were omitted. </details>
… cache Mirrors OfferingsManager.handleErrorFetchingOfferings: when a backend fetch fails and there is no disk cache to fall back on, force the list stale so the next call retries. Previously the old timestamp survived a failed forceRefresh, preventing the retry while the cache appeared fresh. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…serves fresh data forceRefresh=true forced a list re-fetch but left per-workflow detail caches untouched. The subsequent prefetch path called getWorkflow() with staleWhileRevalidate=false, which blocks on a re-fetch when the cache is stale — but returns the cached value immediately when it is still within its 5-minute TTL. A pull-to-refresh triggered within that window would re-fetch the list and silently serve stale detail content from cache. Clear cachedWorkflows before the backend call when forceRefresh=true so every subsequent prefetch is a guaranteed cache miss and fetches fresh data from the backend regardless of the TTL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move clearWorkflowDetailCaches() into the onSuccess handler so detail caches are only evicted after a successful list fetch. Previously the clear happened before the network call, which meant a failed forceRefresh left the detail cache empty with no disk envelopes to restore from. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
vegaro
force-pushed
the
cesar/wfl-280-replace-forceworkflowslistcachestale-with-a
branch
from
10f949b to
a3e28bb
Compare
The test covered a scenario where the in-memory timestamp was fresh but the disk cache returned null — a state that cannot occur in production because cacheWorkflowsList always writes both. The `!restoredFromDisk` branch it exercised is a no-op on the real path: when a forceRefresh fails, the disk copy restores the old list and stamps a fresh timestamp, and self-healing happens via the next forceRefresh from offerings (which bypasses TTL anyway). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
vegaro
deleted the
cesar/wfl-280-replace-forceworkflowslistcachestale-with-a
branch
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
2 participants
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.
Stacked on #3540.
forceWorkflowsListCacheStale()was a side effect I didn't like that cleared the list-cache timestamp, and the nextgetWorkflowsListchecked it. That made it a bit coupled, hard to follow and not synchronized, so I replaced it with aforceRefreshparameter ongetWorkflowsList. The behavior is preserved and a fresh offerings network response passesforceRefresh = trueand a read from disk passesfalse.Note
Medium Risk
Touches offerings delivery gating and workflows caching/prefetch timing; behavior is intended to match the old force-stale path but adds new error-path invalidation and detail-cache clearing on successful force refresh.
Overview
Replaces the
forceWorkflowsListCacheStale()side effect with aforceRefreshargument ongetWorkflowsList, so offerings can request a workflows refresh in one call instead of invalidating the list timestamp first.OfferingsManagerpassesforceRefresh = !loadedFromDiskCache: network offerings trigger a workflows list refetch and realignment of the offeringId → workflowId map; disk fallback keeps the workflows list on its normal TTL.On a successful forced list fetch,
WorkflowManagerclears in-memory workflow detail caches so prefetches always miss TTL and hit the backend. Detail caches are not cleared before the network call, so a failed forced fetch leaves cached details intact. List fetch errors without disk restore now callinvalidateWorkflowsListTimestamp()(renamed fromforceWorkflowsListCacheStale) so the next call retries, matching offerings error behavior.Concurrent callers still dedupe;
forceRefreshis ignored when joining an in-flight batch (documented). Tests updated and expanded forforceRefreshbehavior.Reviewed by Cursor Bugbot for commit d43f8a2. Bugbot is set up for automated code reviews on this repo. Configure here.