do not keep regenerating scripts if already cached

[wintonzheng]

Summary by CodeRabbit

• Bug Fixes
  • Improved cache handling for workflows utilizing external scripts by refining the invalidation logic. Blocks executed through external integrations are now correctly managed in the caching system, ensuring more accurate workflow execution behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

---

Important

Refines caching logic in _execute_workflow_blocks() in service.py to exclude script-backed blocks from caching and expands cache eligibility for other components.

• Behavior:
  • Refines caching logic in _execute_workflow_blocks() in service.py to exclude script-backed blocks from being cached.
  • Expands cache eligibility for other workflow components by checking if block.label is not in script_blocks_by_label.
• Misc:
  • Removes redundant check for disable_cache attribute in _execute_workflow_blocks() in service.py.

^{This description was created by for 275d248. You can customize this summary. It will automatically update as commits are pushed.}

---

---

🔄 This PR optimizes workflow block caching by preventing regeneration of scripts that are already cached, improving performance by avoiding redundant script generation for blocks that have already been processed.

🔍 Detailed Analysis

Key Changes

• Caching Logic: Added check for block.label not in script_blocks_by_label to prevent re-caching already processed blocks
• Cache Control: Removed the disable_cache attribute check, allowing more blocks to be eligible for caching
• Condition Reordering: Moved the script block check earlier in the conditional logic for better performance

Technical Implementation

flowchart TD
    A[Block Execution Complete] --> B{Block has label?}
    B -->|No| E[Skip Caching]
    B -->|Yes| C{Label in script_blocks_by_label?}
    C -->|Yes| E[Skip Caching - Already Cached]
    C -->|No| D{Status == completed?}
    D -->|No| E
    D -->|Yes| F{Block type cacheable?}
    F -->|No| E
    F -->|Yes| G[Add to blocks_to_update]

Loading

Impact

• Performance Improvement: Eliminates redundant script generation for blocks that are already cached in script_blocks_by_label
• Cache Efficiency: Expands caching eligibility by removing the disable_cache restriction while maintaining proper exclusion of already-cached blocks
• Resource Optimization: Reduces computational overhead by avoiding duplicate processing of workflow blocks that have already been converted to scripts

Created with Palmier

[coderabbitai]

Walkthrough

A guard condition is added to the cache-invalidation logic in the workflow service. Blocks with labels present in the script_blocks_by_label mapping are excluded from caching, ensuring externally-executed blocks follow code-driven execution paths rather than relying on cache.

Changes

Cohort / File(s)	Summary
Cache-invalidation guard skyvern/forge/sdk/workflow/service.py	Adds a condition to skip caching blocks whose labels appear in script_blocks_by_label, preventing external script-driven blocks from using the cache-invalidation path

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Attention areas:
  • Verify the guard condition correctly identifies script-driven blocks and doesn't inadvertently exclude legitimate cacheable blocks
  • Confirm the logic flow and interaction with existing block-type eligibility and completion status checks
  • Assess potential side effects on cache behavior and execution flow for blocks with and without script mappings

Possibly related PRs

• cache invalidation update - only delete cached code for impacted block and blocks after the impacted #3908: Introduces CacheInvalidationPlan and selective-clear logic that operates on the same block-selection paths affected by this guard
• add disable_cache to block #3713: Adds block.disable_cache flag for script-driven execution gating, complementary to this label-based exclusion approach
• Refactor script gen with block level code cache #3910: Adjusts block-level caching logic and cache-invalidation propagation, directly related to the same cache paths

Poem

🐰 A cache guard hops near, swift and clear,
"Script-driven blocks shall not appear!"
When labels match the scripts we hold,
We skip the cache, brave and bold.
No phantom data left behind—
Just fresh paths for each block we find. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'do not keep regenerating scripts if already cached' directly aligns with the PR's main objective to prevent script regeneration when cached versions exist.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch shu/do_not_keep_generating_scripts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 275d248 in 1 minute and 3 seconds. Click for details.

• Reviewed 14 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. skyvern/forge/sdk/workflow/service.py:3312

• Draft comment:
  When block_labels are provided and code_gen is false, the function returns early. Ensure this behavior (skipping script regeneration during partial execution) is intended; please add clarification.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. skyvern/forge/sdk/workflow/service.py:3338

• Draft comment:
  The set comparison updates only for missing expected block labels. Consider whether extra (obsolete) labels in the cached script should also trigger regeneration.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. skyvern/forge/sdk/workflow/service.py:3350

• Draft comment:
  Good check to skip regeneration if no update is needed. A brief inline comment explaining the rationale for the should_regenerate logic would improve clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_xWCyJDEno6yH1j23

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 275d248 in 2 minutes and 18 seconds. Click for details.

• Reviewed 14 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. skyvern/forge/sdk/workflow/service.py:968

• Draft comment:
  The caching update condition was changed to remove the check for disable_cache. Previously, blocks with disable_cache=True were skipped, but now they are added to the update set if other conditions hold. Confirm that this change is intended and consider adding a comment explaining why the disable_cache check is removed.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment explicitly asks the PR author to "confirm that this change is intended" which directly violates the rule: "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended". The comment is also asking for a code comment to be added explaining the change, which is somewhat speculative - it assumes that the change needs explanation without strong evidence that it's incorrect. The comment doesn't identify a clear bug or issue, it's just questioning whether the change was intentional. This type of comment should be deleted according to the rules. Perhaps the removal of the disable_cache check could actually be a bug - maybe blocks with disable_cache=True should still be excluded from the update set. Without understanding the full context of what disable_cache means and how it's used, I might be missing something important. Even if this could potentially be a bug, the comment doesn't make a definitive statement about it being wrong. It only asks for confirmation and suggests adding a comment. According to the rules, I should only keep comments where there is STRONG EVIDENCE that the comment is correct. This comment is purely asking for confirmation, which is explicitly prohibited. This comment should be deleted because it asks the PR author to confirm their intention and doesn't identify a clear, definitive issue with the code. It violates the rule against asking for confirmation or explanation.

Workflow ID: wflow_o9sCCNkQEvpzwimh

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}