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.

Co-authored-by: John "codefu" McDole <john@mcdole.org>

…asks_cache

jtmcdole · 2026-09-10T20:30:36Z


+  /// Atomically inserts multiple [entries] into [subcacheName] in a single batch API call
+  /// if and only if their [VersionedCacheEntry.revisionId] is strictly greater than any
+  /// existing cached revision for that key.
+  Future<void> insertVersioned(
+    String subcacheName,
+    List<VersionedCacheEntry> entries,
+  );
+


Question on the description: is it insert All or Nothing if any extra's revision id is not greater than?

jtmcdole · 2026-09-10T20:43:50Z

    orderedPresubmit: OrderedPresubmit.defaultInstance,
    dynamicTestSuppression: false,
    geminiModel: 'gemini-3-flash-preview',
+    taskCachingEnabled: true,


As a matter of hygiene (cocoon roles out early in the AM) - I think this flag should be turned off by default, then enabled in the config.yaml when someone is online. this includes all the places where its defaulted to "true".

jtmcdole · 2026-09-10T20:48:39Z

+/// See [TaskCacheService] for full architectural documentation and caching invariant flowcharts.
 mixin FirestoreQueries {
+  CacheService? get cache => null;
+  Config? get config => null;


config should probably not be null - everywhere else it appears to be set. Maybe this is just a firestore.dart quirk?

jtmcdole · 2026-09-10T20:52:46Z

      );
    }

-    // For tasks, therer is no reason to _not_ order this way.


does your agent hate comments or something? Why was this removed?

(hint: my agent hates comments it didn't write)

jtmcdole · 2026-09-10T20:54:22Z

+    List<Task>? tasks;
+    final docIds = await _taskCache!.getTaskIdsForCommit(commitSha);
+    if (docIds != null && docIds.isNotEmpty) {
+      final lookupResult = await _taskCache!.getTaskPayloads(docIds);


this can and will return null. null! is not good.

jtmcdole · 2026-09-10T21:01:30Z

+      } else if (transform.increment case final inc?) {
+        if (field?.integerValue case final oldVal?) {
+          final newVal = int.parse(oldVal) + int.parse(inc.integerValue!);
+          fields[transform.fieldPath!] = Value(integerValue: newVal.toString());


newVal.toValue()

jtmcdole · 2026-09-10T22:27:13Z

@@ -1213,10 +1213,7 @@ class LuciBuildService {
    task.resetAsRetry();


this changes the document name (_attempt_2?).

Not sure how valid this is, but agent says: "The new retry document ID is never added to tasks_by_commit_ids/$commitSha in Redis. When firestore.dart:354-420 is called, it reads the commit set from Redis (which only contains attempt 1), finds the cached payload, and completely ignores attempt 2 until the 12-hour TTL expire"

vs:

if (createdTasks.isNotEmpty) { await _firestore.updateCacheForCreatedTasks(createdTasks); }

jtmcdole · 2026-09-10T22:28:08Z

-    final firestore = await FirestoreService.from(const GoogleAuthProvider());
+    final firestore = await FirestoreService.from(
+      const GoogleAuthProvider(),
+      cache: cache,


We should probably pass config here?

jtmcdole · 2026-09-10T22:31:04Z

    ]);
+    await _firestore.cacheTaskPayloads(tasksToUpdate);
+    if (createdTasks.isNotEmpty) {
+      await _firestore.updateCacheForCreatedTasks(createdTasks);


just passing through: "Because createdTasks is a subset of tasksToUpdate, and updateCacheForCreatedTasks internally calls cacheTaskPayloads, those created tasks' payloads are serialized and sent to Redis twice in immediate succession."

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

eyebrowsoffire and others added 4 commits September 1, 2026 09:45

Merge branch 'main' into tasks_cache

a43f870

Apply batched suggestions from code review

5bce4ce

Co-authored-by: John "codefu" McDole <john@mcdole.org>

Merge branch 'tasks_cache' of github.com:eyebrowsoffire/cocoon into t…

3ff63e6

…asks_cache

Addressed codefu's comments.

36992cf

eyebrowsoffire requested a review from jtmcdole September 8, 2026 16:36

jtmcdole reviewed Sep 10, 2026

View reviewed changes

jtmcdole requested changes Sep 10, 2026

View reviewed changes

jtmcdole reviewed Sep 10, 2026

View reviewed changes

		@@ -1213,10 +1213,7 @@ class LuciBuildService {
		task.resetAsRetry();

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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