feat(#4840): found critical bug in caching mechanism for transpilation process#4845
Conversation
📝 WalkthroughWalkthroughThe PR enhances the transpilation caching mechanism by introducing contextual metadata (plugin version, tail path, cache directory) into the transpilation logging and pipeline, and expands test coverage with new cache-behavior test cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java`:
- Around line 358-360: The test uses FpDefaultTest.notExistedTarget(...) which
appends "tar/get.txt" and therefore hides the intended transpilation output
path; change the test to use the explicit expected output Path (the resolved
".../fibonacci.xmir") instead of calling notExistedTarget, and ensure its parent
directories are created before use (e.g., createParents on the Path or
Files.createDirectories(target.getParent())); update references to the variable
target and remove reliance on notExistedTarget to ensure the test actually
targets the fibonacci.xmir output.
- Line 405: The test currently ages the directory with
FpDefaultTest.makeOlder(cdir, 1000), which updates the directory mtime (and with
a positive shift can make it newer); instead, change the call to age the actual
cache file mtime into the past — locate the cache file path used in this test
(the cache filename variable or the File/Path pointing to the cache under cdir)
and call FpDefaultTest.makeOlder(cacheFilePath, -1000) (or otherwise pass a
negative shift) so the cache file itself is set to an older mtime and simulates
"cache older than source".
| final Path target = FpDefaultTest.notExistedTarget( | ||
| temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir") | ||
| ); |
There was a problem hiding this comment.
Target path helper hides the intended transpilation output path.
notExistedTarget(...) appends tar/get.txt, so this test doesn’t actually use the .../fibonacci.xmir target and is less representative of real output paths. Consider using the explicit path (and ensure parent dirs exist).
Suggested fix
- final Path target = FpDefaultTest.notExistedTarget(
- temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir")
- );
+ final Path target =
+ temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir");
+ Files.createDirectories(target.getParent());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final Path target = FpDefaultTest.notExistedTarget( | |
| temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir") | |
| ); | |
| final Path target = | |
| temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir"); | |
| Files.createDirectories(target.getParent()); |
🤖 Prompt for AI Agents
In `@eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java` around
lines 358 - 360, The test uses FpDefaultTest.notExistedTarget(...) which appends
"tar/get.txt" and therefore hides the intended transpilation output path; change
the test to use the explicit expected output Path (the resolved
".../fibonacci.xmir") instead of calling notExistedTarget, and ensure its parent
directories are created before use (e.g., createParents on the Path or
Files.createDirectories(target.getParent())); update references to the variable
target and remove reliance on notExistedTarget to ensure the test actually
targets the fibonacci.xmir output.
| Files.createDirectories(cachefile.getParent()); | ||
| final String cached = "transpilation results from cache"; | ||
| Files.write(cachefile, cached.getBytes(StandardCharsets.UTF_8)); | ||
| FpDefaultTest.makeOlder(cdir, 1000); |
There was a problem hiding this comment.
Age the cache file itself (and in the past) for this scenario.
makeOlder(cdir, 1000) updates the directory timestamp (and with a positive shift makes it newer). If the cache decision is based on the cache file’s mtime, this won’t simulate “cache older than source.” Set the cache file mtime to the past instead.
Suggested fix
- FpDefaultTest.makeOlder(cdir, 1000);
+ Files.setLastModifiedTime(
+ cachefile,
+ FileTime.fromMillis(System.currentTimeMillis() - 1_000)
+ );🤖 Prompt for AI Agents
In `@eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java` at line
405, The test currently ages the directory with FpDefaultTest.makeOlder(cdir,
1000), which updates the directory mtime (and with a positive shift can make it
newer); instead, change the call to age the actual cache file mtime into the
past — locate the cache file path used in this test (the cache filename variable
or the File/Path pointing to the cache under cdir) and call
FpDefaultTest.makeOlder(cacheFilePath, -1000) (or otherwise pass a negative
shift) so the cache file itself is set to an older mtime and simulates "cache
older than source".
There was a problem hiding this comment.
Pull request overview
Adds additional coverage and diagnostics around the transpilation caching behavior to help reproduce and investigate issue #4840 (“transpilation doesn't use the cache”).
Changes:
- Added new
FpDefaultcache behavior tests, including a disabled regression test documenting the timestamp-based cache problem. - Enhanced
MjTranspilecache-miss debug logging and slightly refactored repeated expressions into locals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java |
Adds tests targeting transpilation cache miss/hit behavior and documents the suspected timestamp-based flaw. |
eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java |
Improves cache-miss logging with version/hash/tail/cache-dir details to aid debugging cache decisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Files.createDirectories(cachefile.getParent()); | ||
| final String cached = "transpilation results from cache"; | ||
| Files.write(cachefile, cached.getBytes(StandardCharsets.UTF_8)); | ||
| FpDefaultTest.makeOlder(cdir, 1000); |
There was a problem hiding this comment.
In this disabled test, makeOlder(cdir, 1000) only changes the directory timestamp, but FpDefault compares the cache file timestamp (the path returned by CachePath, i.e., cachefile). This means the test setup may not reliably reproduce the intended “cache older than source” scenario when you later enable it. Consider explicitly setting cachefile (and/or source) lastModifiedTime to guarantee cachefile is older than source.
| FpDefaultTest.makeOlder(cdir, 1000); | |
| FpDefaultTest.makeOlder(cachefile, 1000); |
| * than the source files. | ||
| */ | ||
| @Test | ||
| @Disabled |
There was a problem hiding this comment.
@Disabled is used without a reason string, which makes test reports harder to interpret. Consider adding a short explanation directly in the annotation (e.g., referencing #4840 / timestamp-based cache validation) so it’s visible without opening the Javadoc.
| @Disabled | |
| @Disabled("Disabled until #4840: implement proper timestamp-based cache validation") |
| final Path cachefile = cdir | ||
| .resolve("1.0-SNAPSHOT") | ||
| .resolve("94641989dbaebef167cf67906a265d925bbb4e5b") | ||
| .resolve("org/eolang/examples/fibonacci.xmir"); | ||
| Files.createDirectories(cachefile.getParent()); |
There was a problem hiding this comment.
Variable name cachefile is a compound name; per project guideline prefer a single-word noun for locals. Consider renaming it to something like cache/entry (and adjust usages) to keep naming consistent with the guideline.
| Logger.debug( | ||
| this, | ||
| "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)", | ||
| "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss), version: %s, hash: %s, tail: %s, cache enabled: %b, cache dir: %[file]s", | ||
| source, | ||
| MjTranspile.info(source), | ||
| target, | ||
| MjTranspile.info(target), | ||
| System.currentTimeMillis() - start | ||
| System.currentTimeMillis() - start, | ||
| version, | ||
| hsh.get(), | ||
| tail, | ||
| this.cacheEnabled, | ||
| cdir |
There was a problem hiding this comment.
The debug message always says “(cache miss)”, but this lambda can also run when caching is disabled (cache enabled: false). This can be misleading when diagnosing builds. Consider adjusting the wording to reflect both cases (e.g., “cache miss or disabled”) or emitting different messages depending on cacheEnabled.
| final String version = this.plugin.getVersion(); | ||
| final Path tail = base.relativize(target); | ||
| final Path cdir = this.cache.toPath().resolve(MjTranspile.CACHE); | ||
| new FpDefault( |
There was a problem hiding this comment.
Local name cdir is an abbreviated compound; per project guideline prefer a single-word noun for locals. Consider renaming to cache/dir (or similar) to avoid abbreviations and compound naming.
|
@yegor256 I suggest rewriting the caching mechanism - it's full of bugs and it's hard to comprehend the logic behind of it. |
|
@volodya-lombrozo Thanks for the contribution! You've earned +12 points for this: +16 as a basis, +5.35 for your 107 hits-of-code, but -4 for exceeding 100 hits-of-code and -5.35 to cap the HoC bonus at 16 points max. According to our policy, contributions over 100 hits-of-code get penalized to encourage smaller, more focused changes. Please keep them coming, but consider breaking larger contributions into smaller chunks! 🎯 Your running score is +272; don't forget to check your Zerocracy account too. |
This PR adds a few more tests that reveal the critical problem with the current caching mechanism.
Related to #4840
Summary by CodeRabbit
Bug Fixes & Improvements
Tests