Implement lock-free versioned (revisionId) and Set-based Redis caching for /tasks by eyebrowsoffire · Pull Request #5103 · flutter/cocoon

eyebrowsoffire · 2026-07-10T22:12:53Z

To eliminate read bottlenecks against the Firestore /tasks (kTaskCollectionId = 'tasks') collection without distributed locking (tryLock), this change introduces an optimistic, multi-tier caching layer across Cocoon (Task, FirestoreQueries, and CacheService):

Lock-Free Versioned (revisionId) Payload Caching (tasks subcache):
- Every Task document contains a monotonically increasing revisionId integer.
- CacheService.insertVersioned(subcacheName, entries) executes an atomic check-and-set via Redis Lua script (EVAL) verifying entry.revisionId > cachedRevisionId before updating payloads.
- Chunked batching (batchSize = 20) guarantees <0.1ms Lua execution without starving concurrent readers (MGET).
Native Set-Based Commit Task Indexing (tasks_by_commit_ids):
- Replaces monolithic JSON array strings with native Redis Sets (SMEMBERS, SADD) via getSet, updateSet, and addToSetIfExists.
- Leverages two domain invariants:
  1. Immutable commitSha: Task mutations (updateCacheForTaskMutations) never change a commit's task list membership and do not touch or invalidate tasks_by_commit_ids.
  2. Monotonically Increasing Set: Task attempts only grow over time, so unioning IDs via SADD (updateCacheForCreatedTasks) safely converges without lock coordination.
Partial Cache Recovery (_queryTasksByCommitCached):
- When reading commit tasks, _queryTasksByCommitCached retrieves cached task IDs (SMEMBERS) and performs a batch lookup (MGET).
- If individual payloads are expired or missing (missingDocIds), _queryTasksByCommitCached selectively queries getDocument only for missing entries and merges them with foundTasks, eliminating redundant full-commit queries.
Modular & Explicit Cache Handlers (FirestoreQueries):
- _cacheTaskDocuments(tasks): Reusable helper for versioned Task payload insertions.
- _fetchAndCacheCommitTasks(commitSha): Reusable slow-path query helper shared across read-miss and task-creation-miss paths.
- updateCacheForCreatedTasks(tasks) and updateCacheForTaskMutations(writes): Explicit domain handlers replacing generic ad-hoc cache invalidations.
- _subcacheRecentTasksIds and _queryRecentTasksByNameCached removed in favor of _cacheTaskDocuments warming on query execution (~65 lines simplified).

…g for /tasks To eliminate read bottlenecks against the Firestore /tasks (kTaskCollectionId = 'tasks') collection without distributed locking (tryLock), this change introduces an optimistic, multi-tier caching layer across Cocoon (Task, FirestoreQueries, and CacheService): 1. Lock-Free Versioned (revisionId) Payload Caching (tasks subcache): - Every Task document contains a monotonically increasing revisionId integer. - CacheService.insertVersioned(subcacheName, entries) executes an atomic check-and-set via Redis Lua script (EVAL) verifying entry.revisionId > cachedRevisionId before updating payloads. - Chunked batching (batchSize = 20) guarantees <0.1ms Lua execution without starving concurrent readers (MGET). 2. Native Set-Based Commit Task Indexing (tasks_by_commit_ids): - Replaces monolithic JSON array strings with native Redis Sets (SMEMBERS, SADD) via getSet, updateSet, and addToSetIfExists. - Leverages two domain invariants: 1. Immutable commitSha: Task mutations (updateCacheForTaskMutations) never change a commit's task list membership and do not touch or invalidate tasks_by_commit_ids. 2. Monotonically Increasing Set: Task attempts only grow over time, so unioning IDs via SADD (updateCacheForCreatedTasks) safely converges without lock coordination. 3. Partial Cache Recovery (_queryTasksByCommitCached): - When reading commit tasks, _queryTasksByCommitCached retrieves cached task IDs (SMEMBERS) and performs a batch lookup (MGET). - If individual payloads are expired or missing (missingDocIds), _queryTasksByCommitCached selectively queries getDocument only for missing entries and merges them with foundTasks, eliminating redundant full-commit queries. 4. Modular & Explicit Cache Handlers (FirestoreQueries): - _cacheTaskDocuments(tasks): Reusable helper for versioned Task payload insertions. - _fetchAndCacheCommitTasks(commitSha): Reusable slow-path query helper shared across read-miss and task-creation-miss paths. - updateCacheForCreatedTasks(tasks) and updateCacheForTaskMutations(writes): Explicit domain handlers replacing generic ad-hoc cache invalidations. - _subcacheRecentTasksIds and _queryRecentTasksByNameCached removed in favor of _cacheTaskDocuments warming on query execution (~65 lines simplified).

flutter-dashboard · 2026-07-10T22:13:04Z

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

gemini-code-assist

Code Review

This pull request introduces a lock-free, versioned, and set-based caching strategy for Firestore tasks in Cocoon to resolve read bottlenecks. It adds optimistic concurrency tracking via a revisionId on tasks, introduces batch and set operations to the CacheService (with Redis and in-memory implementations), and integrates caching into FirestoreQueries. The review feedback highlights critical issues: a memory leak in Redis where revision IDs are stored in a non-expiring shared hash, incorrect caching behavior when tasks are deleted, and a performance bottleneck caused by fetching missing tasks sequentially in a loop instead of in parallel.

…d Redis updates

jtmcdole · 2026-08-07T17:28:00Z

re-looking at this now.

jtmcdole

This change is still virtually too larger for me to review alone (~1500 lines). I'm not a redis or lua expert, so I had to fall back on some gemini prompts to try and tease out.

jtmcdole · 2026-08-07T18:14:46Z

+    List<VersionedCacheEntry> entries,
+  ) async {
+    if (entries.isEmpty) return;
+    const insertVersionedScript = '''


I am not a LUA person, so I'm totally falling back on Gemini here: keys (tasks/$docId) are passed as the KEYS arguments to EVAL. However, inside insertVersionedScript, the script dynamically constructs and accesses revision keys via Lua string concatenation.

Redis specification dictates that ALL keys accessed by a Lua script MUST be explicitly declared in the KEYS array.

I think we ok if we have only one redis instance? But the robot says "just pass revKey along with key in KEYS, and then use {tasks/$docId}.

Again, I'm not the expert here.

jtmcdole · 2026-08-07T18:15:25Z

+      return values.map((value) {
+        if (value == null) return null;
+        return base64.decode(value as String);
+      }).toList();


Suggested change

return values.map((value) {

if (value == null) return null;

return base64.decode(value as String);

}).toList();

return [

for (final value in values)

value == null ? null : base64.decode(value as String)

];

jtmcdole · 2026-08-07T18:17:44Z

+      local numKeys = tonumber(ARGV[1])
+      for i = 1, numKeys do
+        local key = KEYS[i]
+        local val = ARGV[1 + (i - 1) * 3 + 1]


agian, not a lua person, but should (i - 1) * 3 be pulled out as a local index?

jtmcdole · 2026-08-07T18:19:19Z

+      if (values is! List || values.isEmpty) {
+        return const {};
+      }
+      return values.map((e) => e.toString()).toSet();


Suggested change

return values.map((e) => e.toString()).toSet();

return {for (var e in values) '$e'};

jtmcdole · 2026-08-07T18:22:43Z

+  /// Evicts a deleted task payload from Redis.
+  Future<void> evictTaskPayload(String docId) async {
+    if (!isEnabled) return;
+    await cache.purge('tasks', docId);


Gemini suggests this poisons the cache because:

When a task document is deleted from Firestore, evictTaskPayload(docId) purges tasks/$docId and revisions/tasks/$docId from Redis. However, it does not remove docId from tasks_by_commit_ids/$commitSha.

jtmcdole · 2026-08-07T18:31:58Z

+    if (taskIds.isEmpty || !isEnabled) return false;
+    var allAdded = true;
+    for (final taskId in taskIds) {
+      final added = await cache.addToSetIfExists(


we easily have tasks.length > 100; so we're calling redis 100 times?

The current "addToSetIfExists" only returns true if the set exists, not if the key was added (missing), so you could do something like:

if redis.call("exists", KEYS[1]) == 1 then redis.call("sadd", KEYS[1], unpack(ARGV, 2)) return 1 end return 0

jtmcdole · 2026-08-07T18:33:52Z

+        await _runCommand(
+          (client) => client.send_object([
+            'EVAL',
+            insertVersionedScript,
+            keys.length.toString(),
+            ...keys,
+            ...args,
+          ]),
+        );


Chunking good: +1

you should add this future to "List<Future> chunks" and then use one await chunks.wait;

jtmcdole · 2026-08-07T18:35:36Z

+    if (status != null) {
+      result = result.where((t) => t.status == status).toList();
+    }
+    result.sort((a, b) => b.createTimestamp.compareTo(a.createTimestamp));


if the timestamp is the same, maybe also compare against currentAttempt?

eyebrowsoffire added the CICD Run CI/CD label Jul 10, 2026

gemini-code-assist Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread app_dart/lib/src/service/cache_service.dart

Comment thread app_dart/lib/src/service/cache_service.dart

Comment thread app_dart/lib/src/service/firestore.dart Outdated

Comment thread app_dart/lib/src/service/firestore.dart Outdated

eyebrowsoffire added 2 commits July 10, 2026 15:21

Merge upstream/main into tasks_cache and resolve conflicts

ccb37df

Addressed Gemini comments.

a5a92d1

jtmcdole requested changes Jul 16, 2026

View reviewed changes

eyebrowsoffire added 4 commits July 22, 2026 10:08

Merge remote-tracking branch 'upstream/main' into tasks_cache

2302c02

Refactor task writes to use transactional full documents and versione…

6cb63c6

…d Redis updates

Addressed codefu's comments.

579a7d1

Fix analysis errors.

be02c2a

eyebrowsoffire marked this pull request as ready for review July 23, 2026 20:54

eyebrowsoffire added 2 commits July 23, 2026 13:57

Fix formatting.

2f706ab

Fix formatting.

c837653

eyebrowsoffire requested a review from jtmcdole July 24, 2026 00:14

jtmcdole requested changes Aug 7, 2026

View reviewed changes

	return values.map((e) => e.toString()).toSet();
	return {for (var e in values) '$e'};

Uh oh!

Conversation

eyebrowsoffire commented Jul 10, 2026

Uh oh!

flutter-dashboard Bot commented Jul 10, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtmcdole commented Aug 7, 2026

Uh oh!

jtmcdole left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants