-
Notifications
You must be signed in to change notification settings - Fork 207
feat(#4840): found critical bug in caching mechanism for transpilation process #4845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,26 +173,34 @@ private int transpiled( | |
| final Supplier<String> hsh = new TojoHash(tojo); | ||
| final AtomicBoolean rewrite = new AtomicBoolean(false); | ||
| final Function<XML, XML> transform = this.transpilation(source); | ||
| final String version = this.plugin.getVersion(); | ||
| final Path tail = base.relativize(target); | ||
| final Path cdir = this.cache.toPath().resolve(MjTranspile.CACHE); | ||
| new FpDefault( | ||
| src -> { | ||
| rewrite.compareAndSet(false, true); | ||
| final long start = System.currentTimeMillis(); | ||
| final String res = transform.apply(xmir).toString(); | ||
| 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 | ||
|
Comment on lines
184
to
+196
|
||
| ); | ||
| return res; | ||
| }, | ||
| this.cache.toPath().resolve(MjTranspile.CACHE), | ||
| this.plugin.getVersion(), | ||
| cdir, | ||
| version, | ||
| hsh, | ||
| base.relativize(target), | ||
| tail, | ||
| this.cacheEnabled | ||
| ).apply(source, target); | ||
| return this.javaGenerated(rewrite.get(), target, hsh.get()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||||||||||
| import com.yegor256.Mktmp; | ||||||||||||||
| import com.yegor256.MktmpResolver; | ||||||||||||||
| import java.io.IOException; | ||||||||||||||
| import java.nio.charset.StandardCharsets; | ||||||||||||||
| import java.nio.file.Files; | ||||||||||||||
| import java.nio.file.Path; | ||||||||||||||
| import java.nio.file.Paths; | ||||||||||||||
|
|
@@ -15,6 +16,7 @@ | |||||||||||||
| import org.hamcrest.MatcherAssert; | ||||||||||||||
| import org.hamcrest.Matchers; | ||||||||||||||
| import org.junit.jupiter.api.Assertions; | ||||||||||||||
| import org.junit.jupiter.api.Disabled; | ||||||||||||||
| import org.junit.jupiter.api.Test; | ||||||||||||||
| import org.junit.jupiter.api.extension.ExtendWith; | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -336,6 +338,93 @@ void cachesEvenItIsZeroVersion(@Mktmp final Path temp) throws Exception { | |||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * This test was added to mitigate an issue with the transpilation cache. | ||||||||||||||
| * You can read more about it here: | ||||||||||||||
| * <a href="https://github.com/objectionary/eo/issues/4840">4840</a> | ||||||||||||||
| * If cache is missing, then transpilation should be performed. | ||||||||||||||
| * @param temp Temporary directory | ||||||||||||||
| * @throws Exception If fails | ||||||||||||||
| */ | ||||||||||||||
| @Test | ||||||||||||||
| void transpilesIfCacheMiss(@Mktmp final Path temp) throws Exception { | ||||||||||||||
| final Path cdir = temp.resolve("cache-folder").resolve("transpiled-folder"); | ||||||||||||||
| Files.createDirectories(cdir); | ||||||||||||||
| final String content = "Source content - no cache"; | ||||||||||||||
| final Path source = FpDefaultTest.existedFile( | ||||||||||||||
| temp.resolve("target/eo/1-parse/org/eolang/examples/fibonacci.xmir"), | ||||||||||||||
| "old" | ||||||||||||||
| ); | ||||||||||||||
| final Path target = FpDefaultTest.notExistedTarget( | ||||||||||||||
| temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir") | ||||||||||||||
| ); | ||||||||||||||
|
Comment on lines
+358
to
+360
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Target path helper hides the intended transpilation output path.
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| new FpDefault( | ||||||||||||||
| src -> content, | ||||||||||||||
| cdir, | ||||||||||||||
| "1.0-SNAPSHOT", | ||||||||||||||
| () -> "94641989dbaebef167cf67906a265d925bbb4e5b", | ||||||||||||||
| Paths.get("org/eolang/examples/fibonacci.xmir"), | ||||||||||||||
| true | ||||||||||||||
| ).apply(source, target); | ||||||||||||||
| MatcherAssert.assertThat( | ||||||||||||||
| "We expect a cache miss to trigger transpilation", | ||||||||||||||
| new TextOf(target).asString(), | ||||||||||||||
| Matchers.equalTo(content) | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * We should use the transpilation cache if it existed before transpilation. | ||||||||||||||
| * Current implementation of {@link FpDefault} relies on file timestamps to decide | ||||||||||||||
| * whether to use the cache or not. | ||||||||||||||
| * If the cache file is older than the source file, then the cache is ignored, which is | ||||||||||||||
| * a critical issue for the transpilation step, because the source files are often | ||||||||||||||
| * modified (e.g. by the parser) right before transpilation, making the cache | ||||||||||||||
| * always older than the source files. | ||||||||||||||
| * @param temp Temporary directory | ||||||||||||||
| * @throws Exception If fails | ||||||||||||||
| * @todo #4840:90min Implement proper cache validation mechanism. | ||||||||||||||
| * Currently, FpDefault relies on file timestamps to decide whether to use | ||||||||||||||
| * the cache or not. This approach is not reliable in many cases. | ||||||||||||||
| * For example, in the transpilation step, the source files are often | ||||||||||||||
| * modified right before transpilation, making the cache always older | ||||||||||||||
| * than the source files. | ||||||||||||||
| */ | ||||||||||||||
| @Test | ||||||||||||||
| @Disabled | ||||||||||||||
|
||||||||||||||
| @Disabled | |
| @Disabled("Disabled until #4840: implement proper timestamp-based cache validation") |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local name
cdiris an abbreviated compound; per project guideline prefer a single-word noun for locals. Consider renaming tocache/dir(or similar) to avoid abbreviations and compound naming.