ci: improve cache keys, observability, and node_modules caching strategy#39709
ci: improve cache keys, observability, and node_modules caching strategy#39709
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCI GitHub Actions and custom actions were updated: cache actions upgraded to v5, cache keys separated into Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39709 +/- ##
===========================================
+ Coverage 70.52% 70.55% +0.02%
===========================================
Files 3271 3271
Lines 116804 116804
Branches 21066 21072 +6
===========================================
+ Hits 82376 82410 +34
+ Misses 32379 32335 -44
- Partials 2049 2059 +10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/actions/setup-node/action.yml (1)
67-81: Consider adding version awareness to the mongodb-memory-server cache key.The cache key
mongoms-${{ runner.arch }}-${{ runner.os }}-v1is static and doesn't account for the MongoDB version being downloaded. If the project upgrades mongodb-memory-server or changes the target MongoDB version, the cache may serve stale binaries until manually busted by incrementingv1.Consider including a version identifier (e.g., from a config file or environment variable) in the key to auto-invalidate when versions change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-node/action.yml around lines 67 - 81, Update the mongodb-memory-server cache key to include a version identifier so the cache auto-invalidates when binaries change: replace the static key expression "mongoms-${{ runner.arch }}-${{ runner.os }}-v1" with one that appends a version token (e.g., an input or env var like MONGOMS_VERSION or inputs.mongodb-version), and ensure that the workflow sets that token (or reads it from your config) before the cache step; leave the existing setup that writes MONGOMS_DOWNLOAD_DIR and MONGOMS_PREFER_GLOBAL_PATH to the environment intact so the downloaded binary path is still used..github/workflows/ci.yml (1)
96-109: Robust cache key computation, but error suppression ongit archivemay hide failures.The hash computation logic is well-structured with
set -euo pipefail. However, line 108 suppresses stderr with2>/dev/null:METEOR_HASH=$(git archive HEAD apps/meteor 2>/dev/null | sha256sum | awk '{print $1}')If
apps/meteordoesn't exist orgit archivefails for another reason, the error is silently ignored, andsha256sumwill hash an empty stream, producing a valid but potentially misleading hash. This could cause cache collisions across unrelated branches/states.Consider letting errors propagate or adding explicit validation:
🛠️ Proposed fix to handle git archive errors explicitly
- METEOR_HASH=$(git archive HEAD apps/meteor 2>/dev/null | sha256sum | awk '{print $1}') + if ! git archive HEAD apps/meteor > /tmp/meteor-archive.tar 2>&1; then + echo "::warning::git archive failed for apps/meteor, using fallback" + METEOR_HASH="fallback-$(date +%s)" + else + METEOR_HASH=$(sha256sum /tmp/meteor-archive.tar | awk '{print $1}') + rm -f /tmp/meteor-archive.tar + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 96 - 109, The git archive error is being suppressed which can produce a misleading METEOR_HASH; update the METEOR_HASH computation to fail explicitly or validate the path before archiving: first run a check like git ls-tree --exit-code HEAD apps/meteor (or test -d apps/meteor) and abort with a clear error if it doesn't exist, then run git archive HEAD apps/meteor | sha256sum | awk '{print $1}' without redirecting stderr (remove 2>/dev/null) so failures propagate and the CI job doesn't silently produce an empty hash..github/workflows/ci-code-check.yml (1)
58-60: Consider using$GITHUB_OUTPUTor step summary for better observability.The cache observability steps echo to stdout, which is visible in logs but not easily queryable. If you want these metrics available as step outputs or in the workflow summary, consider:
💡 Option 1: Export as step outputs
- name: Cache observability (typecheck) if: matrix.check == 'ts' - run: echo "typecheck-cache-exact-hit=${{ steps.restore-typecheck.outputs.cache-hit }}" + run: echo "typecheck-cache-exact-hit=${{ steps.restore-typecheck.outputs.cache-hit }}" >> $GITHUB_OUTPUT💡 Option 2: Add to job summary for visibility
- name: Cache observability (typecheck) if: matrix.check == 'ts' - run: echo "typecheck-cache-exact-hit=${{ steps.restore-typecheck.outputs.cache-hit }}" + run: echo "### Cache Status" >> $GITHUB_STEP_SUMMARY && echo "TypeCheck cache hit: ${{ steps.restore-typecheck.outputs.cache-hit }}" >> $GITHUB_STEP_SUMMARYIf the current logging approach meets your observability needs, feel free to keep as-is.
Also applies to: 103-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-code-check.yml around lines 58 - 60, Update the "Cache observability (typecheck)" step so it exports the metric as a proper step output or writes to the job summary instead of only echoing to stdout: capture the value from steps.restore-typecheck.outputs.cache-hit and either export it via $GITHUB_OUTPUT (e.g., emit "typecheck-cache-exact-hit=…" to $GITHUB_OUTPUT) or append a human-readable line to the job summary ($GITHUB_STEP_SUMMARY) for easy querying/visibility; apply the same change to the other mirrored observability step referenced in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci-code-check.yml:
- Line 51: Update all occurrences of the actions/cache subactions from v4 to v5:
replace uses: actions/cache/restore@v4 and uses: actions/cache/save@v4 with
uses: actions/cache/restore@v5 and uses: actions/cache/save@v5 (ensure you
update every instance mentioned in the PR — the restore and save usages at the
four locations — so the workflow consistently uses v5 subactions).
---
Nitpick comments:
In @.github/actions/setup-node/action.yml:
- Around line 67-81: Update the mongodb-memory-server cache key to include a
version identifier so the cache auto-invalidates when binaries change: replace
the static key expression "mongoms-${{ runner.arch }}-${{ runner.os }}-v1" with
one that appends a version token (e.g., an input or env var like MONGOMS_VERSION
or inputs.mongodb-version), and ensure that the workflow sets that token (or
reads it from your config) before the cache step; leave the existing setup that
writes MONGOMS_DOWNLOAD_DIR and MONGOMS_PREFER_GLOBAL_PATH to the environment
intact so the downloaded binary path is still used.
In @.github/workflows/ci-code-check.yml:
- Around line 58-60: Update the "Cache observability (typecheck)" step so it
exports the metric as a proper step output or writes to the job summary instead
of only echoing to stdout: capture the value from
steps.restore-typecheck.outputs.cache-hit and either export it via
$GITHUB_OUTPUT (e.g., emit "typecheck-cache-exact-hit=…" to $GITHUB_OUTPUT) or
append a human-readable line to the job summary ($GITHUB_STEP_SUMMARY) for easy
querying/visibility; apply the same change to the other mirrored observability
step referenced in the diff.
In @.github/workflows/ci.yml:
- Around line 96-109: The git archive error is being suppressed which can
produce a misleading METEOR_HASH; update the METEOR_HASH computation to fail
explicitly or validate the path before archiving: first run a check like git
ls-tree --exit-code HEAD apps/meteor (or test -d apps/meteor) and abort with a
clear error if it doesn't exist, then run git archive HEAD apps/meteor |
sha256sum | awk '{print $1}' without redirecting stderr (remove 2>/dev/null) so
failures propagate and the CI job doesn't silently produce an empty hash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf38026b-8c7f-4622-8b71-1215378e8143
📒 Files selected for processing (4)
.github/actions/meteor-build/action.yml.github/actions/setup-node/action.yml.github/workflows/ci-code-check.yml.github/workflows/ci.yml
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
🔇 Additional comments (8)
.github/actions/setup-node/action.yml (2)
36-38: LGTM: Deno caching with proper key construction and output exposure.The Deno cache implementation correctly:
- Uses a granular key combining OS, arch, version, and lockfile hash
- Exposes
cache-hitas an output for observability in calling workflows- Provides appropriate restore-keys for partial cache restoration
Also applies to: 44-53
60-60: LGTM: Conditional yarn caching via setup-node's built-in feature.Using
actions/setup-node's nativecache: yarnoption wheninputs.cache-modulesis truthy is cleaner than managing a separate cache step for yarn dependencies..github/actions/meteor-build/action.yml (2)
33-33: LGTM: Consistent upgrade to actions/cache@v5.All cache steps are uniformly upgraded from v3/v4 to v5, maintaining consistency across the action. The cache keys and paths remain unchanged, ensuring backward compatibility.
Also applies to: 72-72, 81-81, 90-90
23-26: LGTM: Documentation updated to reflect new composite key scheme.The
source-hashdescription now accurately documents that it expects a composite value combiningpackages-build-cache-keyand the Meteor archive hash, matching the usage inci.ymlline 288..github/workflows/ci.yml (2)
36-37: LGTM: Cache key propagation and composite key construction.The new outputs
packages-build-cache-keyandmeteor-rc-cache-keyare properly:
- Defined in job outputs (lines 36-37)
- Used in the packages-build cache key (line 195)
- Combined for meteor-build's
source-hashinput (line 288)This provides more granular cache invalidation - package changes invalidate packages-build cache, while Meteor-specific changes additionally invalidate the Meteor build cache.
Also applies to: 195-195, 288-288
197-200: LGTM: Cache observability steps for debugging.These echo statements provide useful CI debugging information without affecting build behavior. They'll help diagnose cache hit/miss patterns.
Also applies to: 223-227
.github/workflows/ci-code-check.yml (2)
86-92: LGTM!The save-only-on-develop strategy prevents cache pollution from PR branches while maintaining fresh caches from the main development line. The cache key correctly matches the restore key.
93-116: LGTM with a minor consideration.The ESLint cache pattern is consistent with the TypeScript cache approach and the implementation is correct.
One optional consideration: the cache key uses only
yarn.lockhash, so ESLint configuration changes (e.g.,.eslintrc.*updates) won't invalidate the cache unless dependencies also change. ESLint's incremental cache generally handles this gracefully, but if you encounter stale lint results after config changes, you could extend the key to include config files:key: eslintcache-${{ runner.os }}-${{ hashFiles('yarn.lock', '**/.eslintrc*', '**/eslint.config.*') }}This is optional and likely unnecessary for most workflows.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:102">
P1: The packages build cache key excludes package source files, so CI can reuse stale `/tmp/RocketChat-packages-build.tar.gz` after source-only changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ggazzo
changed the title
|
/jira ARCH-2083 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci-code-check.yml (1)
51-51:⚠️ Potential issue | 🟠 MajorAlign cache subactions to v5 in this workflow.
Line 51, Line 88, Line 96, and Line 113 still use
actions/cache/*@v4. This conflicts with the PR objective to move all cache actions to v5.Suggested patch
- uses: actions/cache/restore@v4 + uses: actions/cache/restore@v5 ... - uses: actions/cache/save@v4 + uses: actions/cache/save@v5 ... - uses: actions/cache/restore@v4 + uses: actions/cache/restore@v5 ... - uses: actions/cache/save@v4 + uses: actions/cache/save@v5#!/bin/bash # Verify any remaining v4 cache subaction usages in this workflow. cat -n .github/workflows/ci-code-check.yml | rg -n 'actions/cache/(restore|save)@v4' # Expected after fix: no matchesAlso applies to: 88-88, 96-96, 113-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-code-check.yml at line 51, Update the remaining cache action usages from v4 to v5: replace every occurrence of the strings "uses: actions/cache/restore@v4" and "uses: actions/cache/save@v4" with their v5 equivalents ("@v5") in the workflow so all cache subactions are aligned to v5; verify no other "actions/cache/*@v4" instances remain by searching for that pattern after edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci-code-check.yml:
- Line 51: Update the remaining cache action usages from v4 to v5: replace every
occurrence of the strings "uses: actions/cache/restore@v4" and "uses:
actions/cache/save@v4" with their v5 equivalents ("@v5") in the workflow so all
cache subactions are aligned to v5; verify no other "actions/cache/*@v4"
instances remain by searching for that pattern after edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45f24400-6cc3-4fc3-aac9-bc9035c0e45c
📒 Files selected for processing (5)
.github/actions/meteor-build/action.yml.github/actions/setup-node/action.yml.github/actions/setup-playwright/action.yml.github/workflows/ci-code-check.yml.github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (2)
- .github/actions/setup-playwright/action.yml
- .github/actions/meteor-build/action.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- .github/actions/setup-node/action.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
🔇 Additional comments (1)
.github/workflows/ci-code-check.yml (1)
58-60: Good cache-hit observability additions.Surfacing exact-hit status for typecheck and eslint improves CI cache debugging with minimal overhead.
Also applies to: 103-105
…e key descriptions - Updated action.yml files for meteor-build and setup-node to use actions/cache@v5. - Enhanced descriptions for cache keys to clarify their purpose. - Modified ci workflows to include observability for cache hits and keys.
…e mongoms binary Drop the ~500 MB actions/cache blob that stored full node_modules (including duplicated .vite and ~190 MB mongodb-memory-server binaries). Instead, rely on setup-node's built-in `cache: yarn` for dependency downloads and add a small dedicated cache for the mongodb-memory-server binary outside node_modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… package.json files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…error handling, and version-aware mongoms cache key Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:99">
P2: `packages-build-cache-key` no longer includes `packages/**/package.json`, so workspace manifest changes can incorrectly hit cache and reuse stale package build artifacts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
2 participants
This pull request updates and improves the caching strategy in GitHub Actions workflows and custom actions for the project. The main focus is on making cache keys more robust and granular, upgrading to newer cache action versions, and improving cache observability for CI runs. These changes help ensure more effective cache usage, reduce redundant builds, and provide better insights into cache performance.
Caching improvements and key changes:
source-hashusage to combine these new keys for better cache hit accuracy (ci.yml,.github/actions/meteor-build/action.yml). [1] [2] [3] [4]Cache action upgrades:
actions/cachefrom v3/v4 to v5 in custom actions and workflows for better performance and reliability (action.yml,ci.yml). [1] [2] [3] [4]Cache observability and reporting:
ci.yml,ci-code-check.yml). [1] [2] [3] [4]Workflow and cache logic enhancements:
setup-node/action.yml,ci-code-check.yml). [1] [2] [3]Minor improvements:
These changes together should result in faster, more reliable CI runs with improved cache hit rates and easier debugging of cache issues.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Task: ARCH-2085