[build-tools] Cache Gradle file access time journal with the build cache - #4221
[build-tools] Cache Gradle file access time journal with the build cache#4221sjchmiela wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4221 +/- ##
==========================================
+ Coverage 63.53% 63.64% +0.11%
==========================================
Files 1028 1028
Lines 47033 47059 +26
Branches 9884 9891 +7
==========================================
+ Hits 29879 29944 +65
+ Misses 17053 17014 -39
Partials 101 101 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates the build-cache save/restore workflow in @expo/build-tools so cache archives preserve per-file mtimes across the copy-to-temp + tar round trip, which is required for Gradle’s mtime-based cache pruning (LRU-style cleanup) to keep working.
Changes:
- Preserve file atime/mtime when copying cache entries into the temporary staging directory prior to creating the tarball.
- Add a Jest test that validates mtimes survive the compress + decompress round trip.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/build-tools/src/steps/functions/saveCache.ts | Preserves timestamps on copied cache files before archiving so Gradle cleanup logic can rely on mtimes. |
| packages/build-tools/src/steps/functions/tests/saveRestoreCache.test.ts | Adds a regression test asserting that mtimes survive cache compress/decompress. |
Suppressed comments (2)
packages/build-tools/src/steps/functions/tests/saveRestoreCache.test.ts:35
- Same as above:
cache-restoreis a fixed path underos.tmpdir(), which can collide across test runs. Usemkdtempfor a unique directory (and avoid the separatemkdir).
const restoreDir = path.join(os.tmpdir(), 'cache-restore');
await fs.promises.mkdir(restoreDir, { recursive: true });
packages/build-tools/src/steps/functions/tests/saveRestoreCache.test.ts:45
- The test currently leaves behind temp directories and the generated archive on success, which can accumulate in CI/dev machines. Consider removing them at the end of the test (or via a
finally/afterEach).
const restoredStat = await fs.promises.stat(path.join(restoreDir, 'entry.txt'));
expect(Math.floor(restoredStat.mtimeMs / 1000)).toBe(Math.floor(mtime.getTime() / 1000));
});
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
79c02fa to
8cbca95
Compare
The build cache cleanup we configure (removeUnusedEntriesAfterDays) decides what to evict from Gradle's file access time journal (caches/journal-1), not from the cache entry files' mtimes. We only saved/restored caches/build-cache-1, so every build started with a fresh journal whose inception timestamp is "now" — making every restored entry look freshly used, so nothing ever aged out and the archive only grew. Save and restore journal-1 alongside build-cache-1 so real per-entry last-used times survive the round trip and cleanup can actually evict stale entries. Both paths also key the cache version, so they must stay in sync between save/restore. Verified against Gradle 8.10.2 running the exact cleanup init script: with only build-cache-1 restored, a 10-day-unused entry is kept; restoring the journal too evicts it, while a recently-used entry is correctly retained. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8cbca95 to
512a144
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/build-tools/src/steps/functions/tests/saveRestoreCache.test.ts:50
- The test leaves temporary directories and the generated tar archive on disk. Cleaning them up avoids leaking /tmp contents and keeps repeated runs stable.
await fs.promises.readFile(
path.join(restoreDir, 'journal-1', 'file-access.properties'),
'utf8'
)
).toBe('inception=1');
});
packages/build-tools/src/steps/functions/tests/saveRestoreCache.test.ts:39
- This test uses fixed paths under os.tmpdir() (e.g. /tmp/gradle-caches), which can collide across parallel test runs and can also pick up leftover files from earlier runs. Use fs.promises.mkdtemp to create unique per-test directories and avoid flakiness.
This issue also appears on line 45 of the same file.
const cachesDir = path.join(os.tmpdir(), 'gradle-caches');
const buildCacheDir = path.join(cachesDir, 'build-cache-1');
const journalDir = path.join(cachesDir, 'journal-1');
await fs.promises.mkdir(buildCacheDir, { recursive: true });
await fs.promises.mkdir(journalDir, { recursive: true });
Complements caching the journal: Gradle's build cache cleanup reads the file access time journal, but for entries the journal has no record of it falls back to max(journalInceptionTimestamp, file.lastModified()). fs.copyFile resets copied entries' mtimes to "now", which would make any such fallback entry look permanently fresh and never age out. Preserve the source mtime so fallback entries can still be evicted; entries tracked in the journal are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: capture the source timestamps before copyFile rather than re-statting after, and tighten the comment.
|
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |
Why
It seems the cache is not getting pruned properly. It looks like Gradle uses journal to keep track of accessed files. Since we're not caching it alongside build cache, Gradle defaults all files to "they have been used now".
How
When caching Gradle build cache, cache journal too.
Test Plan
Verified locally.