Conversation
Implemented the `feat-task-caching` proposal: - Added `TaskCacheConfig` to `TaskStep`. - Created `ICacheProvider` and `MemoryCacheProvider`. - Added `CachingExecutionStrategy` to wrap inner strategies and skip execution on cache hit. - Updated `TaskRunner` and `TaskRunnerBuilder` with `withCache()` configuration. - Archived OpenSpec proposal to `archive/2026-04-03-feat-task-caching`. Co-authored-by: thalesraymond <32554150+thalesraymond@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a task caching mechanism to the task runner system, including a new CachingExecutionStrategy, a TaskCacheConfig interface for individual steps, and a default MemoryCacheProvider. The implementation allows for skipping task execution on cache hits while optionally restoring context state. Feedback focuses on ensuring that cached results maintain their original success status to prevent breaking dependency chains, addressing potential cache corruption from shallow-copied objects, and mitigating memory leaks in the memory cache provider through better eviction strategies.
| return { | ||
| ...cachedResult, | ||
| status: "skipped", // Or potentially a new 'cached' status, but sticking to existing ones | ||
| }; |
There was a problem hiding this comment.
Overriding the status to "skipped" for cached results is problematic. In many workflow engines, a "skipped" status (often used when a task's condition is false) prevents dependent tasks from executing. Since only successful results are cached (see line 39), the cached result's status is guaranteed to be "success". Overriding it to "skipped" will likely break the dependency chain. It is better to preserve the original status from the cached result.
return {
...cachedResult,
};| const cachedResult = await this.cacheProvider.get(cacheKey); | ||
|
|
||
| if (cachedResult) { | ||
| if (step.cache.restore) { | ||
| await step.cache.restore(context, cachedResult); | ||
| } | ||
| return { | ||
| ...cachedResult, | ||
| status: "skipped", // Or potentially a new 'cached' status, but sticking to existing ones | ||
| }; | ||
| } |
There was a problem hiding this comment.
The cachedResult is returned as a shallow copy. If the data property contains an object, it will be shared between the cache and the current execution context. If subsequent tasks or the restore function modify this object, the cache will be corrupted. Consider deep-cloning the result before returning it, or documenting that cached data must be treated as immutable. Note that if deep-cloning is implemented using JSON.stringify, consider the performance impact as per repository guidelines which favor avoiding expensive operations unless benchmarked.
References
- Retaining separate logic or avoiding expensive operations like JSON.stringify can be justified for performance.
| * An in-memory implementation of ICacheProvider. | ||
| */ | ||
| export class MemoryCacheProvider implements ICacheProvider { | ||
| private cache = new Map<string, CacheEntry>(); |
There was a problem hiding this comment.
The MemoryCacheProvider only evicts expired entries when they are explicitly accessed via get(). In long-running processes where many unique cache keys are generated but never reused, this can lead to a memory leak as expired entries will remain in the Map indefinitely. While using a Map is correct for O(1) lookups, consider implementing a periodic cleanup mechanism (e.g., using setInterval) or using a cache with a maximum size and an eviction policy (like LRU).
References
- Use a Map to ensure O(1) access time for frequent lookups by a specific key.
1 participant
Implemented the task output caching specification to skip redundant task execution.
PR created automatically by Jules for task 9591858606130206977 started by @thalesraymond