fix(workflows): Don't leak the previous user's workflows after a login or logout#6944
Merged
Conversation
#6917 dropped a late list fetch from the previous user on identity change, but a narrow window remained between the success-path generation re-check and the still-unguarded cache(workflowsList:) write. Fold the check into a generation-guarded cache(workflowsList:ifGeneration:) that does the check, the memory and disk writes, and the detail prune atomically under detailsLock, and put restoreWorkflowsListFromDisk and the list clear under the same lock. The guarded write reports whether it landed so the manager skips prefetching a dropped list's details. Scope is the workflows cache only; the offerings cache is intentionally left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 5201a6c. Configure here.
vegaro
reviewed
Jun 9, 2026
vegaro
reviewed
Jun 9, 2026
getWorkflow re-captured currentCacheGeneration() at its own call time. From the prefetch path that's too late: a clearCache() landing between caching the list and the prefetch issuing its getWorkflow makes that fresh capture read the already-bumped generation, so the in-memory write passes its own guard and the previous user's (user-scoped) detail repopulates the cleared cache. The disk write was already correct because it used the list's generation. Forward the list generation into getWorkflow's in-memory write (on-demand callers still capture fresh via the nil default), so both the disk and in-memory writes are guarded by the same token. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vegaro
approved these changes
Jun 9, 2026
vegaro
approved these changes
Jun 9, 2026
Address review: the standalone pruneWorkflowDetails wrapper was dead (the guarded list write is the only pruner and already holds detailsLock), and the "...Locked" suffix on the body read as if it took the lock. Collapse to a single pruneWorkflowDetails that documents the lock precondition instead of encoding a misleading suffix in the name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Follow-up to #6917.
What
#6917 added a cross-user generation guard to the workflow detail cache, and a partial guard on the list fetch (it drops a late list response from the previous user on identity change). But a narrow window remained: between the success-path
currentCacheGeneration() == generationre-check and the actual, still-unguardedcache(workflowsList:)write, aclearCache()on a login/logout could land and the previous user'sofferingId → workflowIdmap would repopulate the cleared cache.That map is the user-targeted part (targeting/experiments can map the same offering to a different workflow per user), so the residual matters even though it self-heals on the next offerings refresh.
This folds the check into the write so there's no window:
cache(workflowsList:ifGeneration:)does the generation check, the in-memory + disk writes, and the detail prune atomically underdetailsLock, and returns whether it landed. The manager's separate "re-check then write" collapses into that one guarded call, and it skips prefetching the details of a dropped list.restoreWorkflowsListFromDisk()and the list clear inclearCache()now run under the same lock too, so clear / write / restore are all mutually atomic (one generation bump covers list + details).pruneWorkflowDetailsinto a locked wrapper + a lock-free…Lockedbody, since the guarded write already holds the non-recursiveLockand would otherwise self-deadlock.forceWorkflowsListCacheStale()stays lock-free on purpose: it only marks an existing entry stale, it never writes a new list, so it can't reintroduce a previous user's map.The race, and why the lock closes it
The fix works because the list write and
clearCache()both serialize ondetailsLock, so the generation check and the write can't be split by a clear. Either the clear lands first (write sees the bumped generation and drops) or the write lands first (the clear wipes it right after). No interleaving in between.sequenceDiagram participant M as WorkflowManager participant C as WorkflowsCache participant ID as IdentityManager Note over M: getWorkflowsList(user A) issued M->>C: currentCacheGeneration() → G Note over M: backend.getWorkflows(A) in flight… ID->>C: login/logout → clearCache() Note over C: under detailsLock:<br/>generation G → G+1,<br/>list + details cleared Note over M: A's list response arrives M->>C: cache(workflowsList: A, ifGeneration: G) Note over C: under detailsLock:<br/>G ≠ G+1 → drop write, return false C-->>M: false M->>M: skip prefetch, fire onComplete Note over C: user B's cache stays cleanBefore this PR, the manager did
currentCacheGeneration() == generationand thencache(workflowsList:)as two separate steps, so aclearCache()landing between them would write A'sofferingId → workflowIdmap into B's freshly cleared cache.Plus: the prefetched detail's in-memory write had the same split
The list write and the prefetched detail's disk write are both guarded by the generation captured when the list fetch was issued, so a
clearCache()correctly drops them. The matching in-memory detail write wasn't:getWorkflowre-capturedcurrentCacheGeneration()at its own call time. From the prefetch path that read happens after the list is cached, so aclearCache()landing in that window makes the capture read the already-bumped generation, the write passes its own guard (new == new), and the previous user's (user-scoped) detail repopulates the cleared in-memory cache.getWorkflownow takes an optionalifGeneration:; prefetch forwards the list's generation so the in-memory and disk writes share one token. On-demand callers passniland keep capturing fresh (their request really is issued at that moment), so their behavior is unchanged.Scope
Workflows cache only. The offerings cache (
DeviceCache.offeringsCachedObject) is intentionally left as-is.AI session context
Metadata
facu/workflow-list-cache-guardGoal
Close the cross-user generation-guard gaps the workflows cache still had after #6917, scoped to the workflows cache only (offerings explicitly out of scope). Two gaps: the list-cache check-then-write window, and the prefetched detail's in-memory write re-capturing the generation instead of using the list's.
Initial Prompt
Two threads. (1) The list-cache window was a follow-up flagged during #6917 review (vegaro: "the list of workflows should probably be protected with a similar lock from cross-user leaks… Can come in a future PR"); the human asked to see the workflows-only fix and confirmed "we must only touch workflows." (2) On a fresh pass over the diff, the human identified that
getWorkflowdoinglet generation = self.workflowsCache.currentCacheGeneration()was itself the bug for the prefetch in-memory write, and asked how to fix it.Important Follow-up Prompts
Agent Contribution
cache(workflowsList:ifGeneration:) -> Bool(generation check + memory/disk write + prune underdetailsLock); movedrestoreWorkflowsListFromDisk()and the list clear under the lock; splitpruneWorkflowDetailsinto a locked wrapper + lock-free body; gated prefetch on the guarded write's result.ifGeneration: Int? = niltoWorkflowManager.getWorkflow; the in-memory write now usesgeneration ?? currentCacheGeneration();prefetchWorkflowsforwards the list's generation so the disk and in-memory writes share one token. Updated the stale doc comment that claimed eachgetWorkflow"guards its own in-memory write the same way".testPrefetchInMemoryWriteIsGuardedByListGenerationNotAFreshCapture, watched it fail (write landed) before the fix and pass after. Earlier list-cache work added aseedWorkflowsListhelper + two guard tests.pruneWorkflowDetailswrapper and the lock-freepruneWorkflowDetailsLockedbody into a singlepruneWorkflowDetailswhose doc states thedetailsLockprecondition, instead of encoding a misleadingLockedsuffix in the name.Human Decisions
Key Implementation Decisions
clearCache()(no check-then-write window).getWorkflow.cache(workflowsList:ifGeneration:)already validated against; a re-capture reads an already-bumped value if a clear landed in between. On-demand callers keep the fresh capture via thenildefault, so their behavior is unchanged.workflowId(forOfferingId:)/ the getters take the lock — they are intentionally lock-free reads; the guarantee is eventual consistency, not serializing every getter.Files / Symbols Touched
Sources/Caching/WorkflowsCache.swift—cache(workflowsList:ifGeneration:),pruneWorkflowDetails(single lock-free body, caller holdsdetailsLock),restoreWorkflowsListFromDisk(under lock),clearCache(list clear under lock).Sources/Purchasing/WorkflowManager.swift—getWorkflow(…, ifGeneration:)and its in-memory write;prefetchWorkflowsforwards the list generation.Tests/UnitTests/Caching/WorkflowsCacheTests.swift—seedWorkflowsListhelper, call-site migration, two guard tests.Tests/UnitTests/Purchasing/WorkflowManagerTests.swift—testPrefetchInMemoryWriteIsGuardedByListGenerationNotAFreshCapture.Dependencies / Config / Migrations
WorkflowManageris internal; the added parameter is optional, so no publicswiftinterfacechange.Validation
xcodebuild test … -only-testing:UnitTests/WorkflowManagerTests -only-testing:UnitTests/WorkflowsCacheTests(iPhone 15 sim): 84 passing, 0 failures (42 + 42).swiftlinton changed files: no violations.Validation Gaps
getWorkflowdirectly with the list's generation, so it coversgetWorkflowhonoring a forwarded generation. The prefetch call site forwarding that generation is verified by inspection: the clear landing between caching the list and the prefetch's synchronous generation capture is not deterministically raceable without a production-only test seam, which was not added.Review Focus
detailsLock → deviceCache(no inversion); the prune split avoids self-deadlock on the non-recursive lock.clearCache/ guarded write / restore are mutually atomic.getWorkflow'sifGenerationdefault (nil→ fresh capture) preserves on-demand behavior; only prefetch forwards the list generation.Risks / Reviewer Notes
workflowId(forOfferingId:)is an intentionally lock-free read, so a clear and a read can still race.clearCache; the guarantee is eventual consistency, and during an identity switch the new user's data doesn't exist yet.Non-goals / Out of Scope
Omitted Context