fix(#5720): share one ConcurrentCache across files so locking works

[anuragpaul602-netizen]

Closes #5720.

Problem

ConcurrentCache documents itself as making Cache thread-safe via a ConcurrentMap<Path, ReentrantLock> keyed by cache tail path. But at all three call sites — Parsing.parsed(), Transpiling.transpiled(), Linting.lintOne() — a new instance was created per file:

new ConcurrentCache(new Cache(...)).apply(...)

Each file therefore got its own empty lock map, so two Threaded workers racing on the same cache tail path each acquired their own uncontended lock and never serialized. The (correct) internal locking from #4900 was defeated by the call-site wiring.

Fix

• ConcurrentCache now holds only the shared lock map; the per-file Cache (which legitimately differs per file) is passed into apply(cache, source, target, tail).
• Parsing, Transpiling, and Linting each keep a single private final ConcurrentCache guard = new ConcurrentCache() field, reused across every file processed by the Threaded pool, so writes to the same tail path share one lock.

Tests

ConcurrentCacheTest updated to the new API and passing.

🤖 Generated with Claude Code

[anuragpaul602-netizen]

Ready for review. This fixes the call-site wiring that #4900 didn't touch: ConcurrentCache was constructed fresh per file, so every thread got its own empty lock map and same-tail writes never actually serialized. Here ConcurrentCache holds only the shared lock map, and each Step (Parsing/Transpiling/Linting) keeps a single reused instance, passing the per-file Cache into apply(...).

Verified locally: ConcurrentCacheTest passes (updated to the new API). Happy to adjust the guard field naming/placement if a different convention is preferred.

[github-actions]

⚠️ Benchmark Comparison Unavailable

Unfortunately, one of the benchmarks is missing, and we couldn't generate a performance comparison report.

Please ensure that both the base and PR benchmark results are available for analysis.

[yegor256]

@anuragpaul602-netizen the qulice fails here, please fix

[morphqdd]

Reviewed by checking out the branch, reading the full diff, and running ConcurrentCacheTest, MjParseTest, MjTranspileTest, CompilingTest, LintingTest (all green) plus the full mvn clean verify -Pqulice gate (clean). The fix itself correctly targets the root cause of #5720. Main concern, detailed inline: the test suite doesn't actually exercise the fix at the level where the original bug lived.

[morphqdd]

Correct diagnosis in the doc: the old code's locks map was fresh (and empty) per ConcurrentCache instance, and instances were being created per file, so two threads racing on the same tail path never actually shared a lock. This comment correctly identifies why a single shared instance is required.

[morphqdd]

This is the actual root-cause fix: the no-arg constructor no longer takes a Cache, meaning one ConcurrentCache (and its one locks map) can now be built once and reused for every file, instead of being thrown away and rebuilt per file.

[morphqdd]

Right call to make cache a parameter of apply() rather than part of the constructor: the per-file Cache (the actual compilation lambda) genuinely differs per file, while the locks map must not. Separating "what varies per file" from "what must be shared across files" is exactly the fix this bug needed.

[morphqdd]

Confirmed correct: the per-file cache.apply(...) call happens inside the lock acquired from the shared locks map keyed by tail, so two threads now genuinely serialize on the same lock object for the same tail path.

[morphqdd]

Correct: one guard field per Linting instance, matching the fix's requirement that a single ConcurrentCache be shared across every file this step processes.

[morphqdd]

This is where the fix actually takes effect for linting: this.guard.apply(...) instead of new ConcurrentCache(...).apply(...). Verified via mvn -pl eo-maven-plugin test -Dtest=LintingTest, though see my note on ConcurrentCacheTest.java -- nothing currently drives two concurrent calls through this exact call site to prove the sharing works end to end.

[morphqdd]

Same pattern applied consistently here for parsing. Good -- all three call sites (Linting, Parsing, Transpiling) get the fix, not just one.

[morphqdd]

And here for transpiling too. Confirmed via mvn -pl eo-maven-plugin test -Dtest=MjTranspileTest (40/40 green).

[morphqdd]

This is my main concern with the PR. This test was updated only mechanically (passing cache/original as a parameter instead of via the constructor), but it still constructs exactly one ConcurrentCache and hits it with 100 threads for the same tail path -- that would have passed identically against the old, buggy code too, since the original bug only manifested when different ConcurrentCache instances got created per file, which this test never does, before or after the fix. None of LintingTest (one unrelated test, skipsLintingWhenFlagIsSet), MjParseTest, or MjTranspileTest drive two concurrent calls to lintOne/parsed/transpiled for the same target path through a shared Linting/Parsing/Transpiling instance to prove this.guard is doing its job end to end. Right now the only thing standing between "fixed" and "silently still broken at the call site" is code reading, not a test. Would like to see at least one test that drives a Step with two threads targeting the same tail path and asserts the underlying work runs once, before merge.

[morphqdd]

The fix itself (holding one ConcurrentCache guard field per Parsing/Transpiling/Linting instance instead of a fresh instance per file) is correct for the specific race described in #5720: two Threaded workers on the same instance racing on the same tail path. Three things beyond that scope, verified by reading the surrounding code, worth resolving before this closes the issue:

1. MjSafe.assembling() (MjSafe.java, inside the assembling() method) creates a second, independent Parsing instance, separate from the one MjParse.exec() creates directly. ConcurrentCache's own new javadoc states the invariant this fix relies on: "a single instance must be shared across all files of one Mojo run." That invariant holds within one Parsing instance, but Parsing is instantiated in at least two places (MjParse.java and MjSafe.assembling(), the latter used by MjAssemble and MjCompile), each getting its own separate guard and lock map.

Within one module's sequential build this is fine, since these Mojos run at different lifecycle phases, not concurrently with each other. But under a parallel reactor build (mvn -T N, a common and supported mode) with the default cache directory, which is machine-wide rather than module-scoped (already flagged as issue #2857, still open), two different modules building in parallel could each spin up their own Parsing instance and still race on the same cache tail path if they happen to touch the same dependency. This PR closes the within-one-instance case but doesn't mention or address the cross-instance case. Worth at least a note in the javadoc about this residual gap, if not a fix tying the two together (a shared static lock map, or threading the guard down from a single owner).

2. Linting.lintAll() writes to the WPA package-level cache entry via a raw new Cache(...).apply(...), bypassing ConcurrentCache/this.guard entirely. It's the one Cache call site in this class that isn't guarded. It's called once per lint() run, outside the per-file Threaded loop in lintOne(), so it's probably not racing against anything today, but nothing here says that, and it reads as an oversight next to the other three call sites that were all moved onto the shared guard. Either route it through this.guard.apply(...) too for consistency, or add a comment explaining why this specific write doesn't need it.

3. No test exercises the actual bug this PR fixes. I checked the existing concurrency tests: MjParseTest.parsesConcurrentlyWithLotsOfPrograms and MjTranspileTest.transpilesSeveralEoProgramsInParallel both spin up 30-50 different files, each with its own tail path, never two operations racing on the identical tail path, so they'd pass whether or not #5720 were fixed. ConcurrentCacheTest only exercises the ConcurrentCache primitive directly, which the PR description says was already correct before this change (per #4900); the bug was purely in the call-site wiring. Nothing tests that Parsing, Transpiling, or Linting actually share one guard instance across multiple calls, the exact thing this PR changes. A regression test that would have caught the original bug needs to exercise one of those three classes directly: two concurrent calls with the same tail path through the same instance, asserting the underlying compilation function runs once, not twice.

[morphqdd]

A few more, anchored to the actual changed lines.

[morphqdd]

This field's javadoc ("Shared cache guard, reused across all files so that concurrent writes to the same cache tail path are actually serialized (#5720).") is copied verbatim into Parsing.java and Transpiling.java too. Three identical multi-line doc comments for the same concept is the kind of duplication worth avoiding, since a future change to the explanation (or to the invariant it documents, see the assembling() comment on the main review) now has to be kept in sync by hand in three places. Worth centralizing, e.g. on ConcurrentCache's own class javadoc (which already explains the sharing requirement) and just pointing to {@link ConcurrentCache} from each field here instead of repeating the explanation.

[anuragpaul602-netizen]

Done in bff3613. All three fields (Parsing, Transpiling, Linting) now carry the same one-liner:

/**
 * Cache guard, see {@link ConcurrentCache} for why it is one per instance.
 */
private final ConcurrentCache guard;

The explanation now exists once, on ConcurrentCache's class javadoc — which is also where the residual cross-instance gap you raised in the main review is now recorded. So a future change to the invariant, or to what it does and doesn't cover, is one edit rather than three that have to be kept in sync.

[morphqdd]

apply(Cache cache, Path source, Path target, Path tail) puts the newly-added cache parameter first, ahead of the established source, target, tail triple. Cache.apply(source, target, tail) itself (the method this delegates to) keeps that triple as its own full signature. Putting cache last here, apply(source, target, tail, cache), would match the wrapped method's own parameter order and read as "the same call, plus which cache to use" rather than breaking the familiar order at the front.

[anuragpaul602-netizen]

Done in bff3613 — the signature is now apply(Path source, Path target, Path tail, Cache cache), with the javadoc @param order following it.

You're right that it reads better: the wrapped call keeps its own full signature at the front and the cache is what's appended. It also happens to lay out better at the call sites, because the multi-line new Cache(...) argument is now last instead of pushing the three paths onto a trailing line:

this.guard.apply(
    source, target, base.relativize(target),
    new Cache(
        new CachePath(...),
        src -> ...
    )
);

[morphqdd]

This still only tests the ConcurrentCache primitive directly, which per the PR description was already correct before this change (the bug was in the call-site wiring, not in this class's own locking). Nothing in this file or elsewhere in the diff tests that Parsing, Transpiling, or Linting actually share one guard instance across multiple calls now, which is the actual fix. A test at that level, e.g. calling the package-private parsed/transpiled/lintOne twice concurrently with the same tail path through one Parsing/Transpiling/Linting instance and asserting the wrapped compilation runs once, would have caught the original bug and would catch a regression if the wiring changes again.

[anuragpaul602-netizen]

Agreed, and fixed in bff3613 — this file stays a test of the primitive (only the argument order changed), and the actual regression test is the new ParsingTest:

new Threaded<>(
    Collections.nCopies(8, tojo),
    each -> { parsing.parsed(each, countingPipeline, "digest"); return 1; }
).total();
MatcherAssert.assertThat(..., parses.get(), Matchers.equalTo(1));

Eight threads go through one Parsing instance, with the same tojo — so the same name, the same target, and therefore the identical tail path. The pipeline handed in counts its own invocations. With the guard shared by the instance, the first thread compiles and the other seven wait on its lock and then read what it cached, so the count is exactly 1 and it is deterministic, not a lucky interleaving. With a guard per file, each thread locks its own empty map, all eight miss, and the source is parsed up to eight times.

I checked it fails on the bug rather than just passing on the fix: reverting this.guard.apply(...) back to new ConcurrentCache().apply(...) in Parsing alone fails the test on 3 out of 3 runs. Parsing.parsed(TjForeign, UnaryOperator, String) had to become package-private for the test to drive it — the javadoc says why.

Two caveats worth stating plainly:

• It covers Parsing only. Transpiling.transpiled and Linting.lintOne have the identical shape, and I'd rather have one honest reproduction than three copies of it. Say the word if you want all three.
• It swallows one exception on purpose. Saved truncates the target and streams into it, so when several threads legitimately share a target file, a thread reading target back for tojoVersion/Xnav can catch it half-written and get Premature end of file. That read sits after guard.apply returns, outside the guarded region, so it's a distinct race that this PR doesn't claim to fix — the test catches it, counts it, and asserts only on the parse count. It's real though (I hit it while writing the test), and I'm happy to open a separate issue for it if you agree it's worth one.

[anuragpaul602-netizen]

Opened it as #5873, and ParsingTest now names it: the catch carries a Logger.debug(..., "#5873") and the method javadoc says why it's there, so it doesn't read as a swallowed failure. It can go away once #5873 is fixed.

The issue proposes making Saved write atomically (stream into a sibling temp file, then Files.move(..., ATOMIC_MOVE)) rather than widening the guarded region in each of the three classes — the invariant belongs to whoever writes the file, not to every caller who reads it back, and it also covers readers other than these three. Pushed as a993353.

[morphqdd]

Confirming the verdict: request changes, per the two reviews above (the cross-instance Parsing sharing gap via MjSafe.assembling(), the unguarded WPA cache write in Linting.lintAll(), the missing regression test at the actual wiring level, plus the three smaller inline points on the duplicated field javadoc, the apply() parameter order, and the test file).

[anuragpaul602-netizen]

Thanks for the careful read — all six points are addressed in bff3613. Replies to the three inline threads are in place; here are the three from the main review.

1. MjSafe.assembling() and the cross-instance gap

You're right, and I've documented it rather than fixed it. ConcurrentCache's class javadoc now states what the invariant does not cover:

The serialization is in-instance only: every owner (Parsing, Transpiling, Linting) keeps its own guard, and Parsing alone is instantiated both by MjParse and by MjSafe#assembling(). Within one module that is enough, because those mojos run in different lifecycle phases and never overlap. Two modules built at the same time (a parallel reactor, or two separate Maven processes) still write to the same cache location whenever they touch the same dependency, because the default cache directory is machine-wide rather than module-scoped (#2857); no in-process lock can serialize that, so it stays out of scope here.

The reasoning for documenting instead of fixing: threading the guard down from a single owner can't work, because MjParse and MjAssemble/MjCompile are separate mojo objects with no common owner to thread it from. That leaves a static lock map, which would serialize mvn -T within one JVM but does nothing for two separate mvn processes sharing ~/.eo — the same #2857 scenario, and the one CI hits most. So it buys a partial fix in exchange for permanent static mutable state in a codebase that otherwise has none (Catalogs.INSTANCE is the only precedent, and it's immutable). Documenting the boundary honestly seemed the better trade for a PR scoped to #5720.

Entirely your call, though — if you'd rather have the reactor case covered, say so and I'll add the static map here.

2. The unguarded WPA write in Linting.lintAll()

Fixed — it goes through this.guard now, so all four Cache call sites in the class are consistent:

this.guard.apply(
    this.sourcesDir, target, wpa,
    new Cache(
        this.cacheDir.resolve(Linting.CACHE),
        root -> { ... },
        p -> p.getFileName().toString().endsWith(".xmir") && !p.getFileName().equals(wpa)
    )
);

Your read of it was right — it's called once per lint(), outside the per-file Threaded loop, so it isn't racing today. But it's a write to a cache tail path like any other, and "safe because of where it happens to be called from" is exactly the kind of thing that stops being true after a refactor. Routing it through the guard costs nothing (uncontended ReentrantLock) and removes the need for a comment explaining the exception.

3. A test that actually catches the bug

Added ParsingTest.parsesOnlyOnceWhenThreadsRaceOnTheSameTail: eight threads through one Parsing instance on the identical tail path, asserting the parse pipeline is invoked exactly once. Details and two caveats are in the inline reply — the short version is that it's deterministic on the fix, and reverting Parsing alone to new ConcurrentCache().apply(...) fails it 3 out of 3 runs. It covers Parsing only, and it deliberately tolerates a separate race in the post-guard target read that this PR doesn't fix; both are explained in that reply.

Verification

• Full eo-maven-plugin suite: 416 tests, 0 failures, 5 skipped.
• Qulice: 61 violations, byte-identical to the pre-change baseline on this branch (the 3 in Transpiling are pre-existing). Making Parsing.parsed package-private tripped MethodsOrderCheck, so it moved above the private overloads.
• Javadoc builds clean — no new warnings from the touched classes.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud