Skip to content

Fix/compile cache self reference - #84

Merged
sake92 merged 4 commits into
mainfrom
fix/compile-cache-self-reference
Jun 14, 2026
Merged

Fix/compile cache self reference#84
sake92 merged 4 commits into
mainfrom
fix/compile-cache-self-reference

Conversation

@sake92

@sake92 sake92 commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • Documentation

    • Added warnings against task anti-patterns that break caching.
    • Clarified how caching behavior works and how to avoid self-referential dependencies.
  • Tests

    • Added integration test to verify caching functions correctly.
  • Bug Fixes

    • Fixed task dependency configuration to improve cache hit rates.

sake92 and others added 4 commits June 14, 2026 16:44
… inputs

Documents the filesystem-feedback (not graph-cycle) self-reference that keeps
compile from cache-hitting and re-hashes the class tree every build. Adds the
rule to AGENTS.md and a caching.md anti-pattern section; fixes stale 'compile
always runs' claims (compile is a CachedTask now).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The pinned v0.17.0 config schema no longer parses the current deder.pkl
(Pkl ConversionException on cold server start), breaking the self-build.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
compile's cache key transitively included its own output dirs, whose contents
it writes — making the key change every build so it never hit (re-running Zinc
and re-hashing ~14k class files on every no-op):
- via compileClasspath -> allClassesDirs -> classes (own classes dir)
- semanticdbDir (semanticdb output)
- generatedSources (javac annotation-processing output)

These are filesystem feedback loops, not graph cycles, so cycle detection
never caught them; non-deterministic .class/.semanticdb bytes compounded it.

Fix: compile no longer depends on those output dirs. It derives their paths at
execute time (ctx.out/classes, etc.) and builds its compile classpath from the
dependency coordinates + upstream modules' transitive compile outputs + its own
(derived) classes dir for javac annotation processing. compileClasspathTask is
untouched, so runClasspath/BSP/publish keep the full classpath.

Verified on deder itself (deder-common, plugin-api) and sample-projects/multi:
compile now hits on an unchanged second run. Adds a CachedTaskSuite regression
test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

compileTask is refactored to remove self-referential dependencies on generatedSourcesTask, compileClasspathTask, and semanticdbDirTask (which content-hashed the task's own output directories), replacing them with upstream dependency resolution tasks and deriving output paths from ctx.out at execute time. An integration test verifies the resulting cache hit, and both caching.md and AGENTS.md document the anti-pattern and rules to avoid it.

Changes

Compile Cache Self-Reference Fix

Layer / File(s) Summary
Anti-pattern documentation and contributor guidance
docs/content/reference/caching.md, AGENTS.md
caching.md adds a new "Anti-pattern: never content-hash a task's own outputs" section, updates the hash-chain diagram to show compile as a cacheable task, and rewrites the description of compile caching. AGENTS.md adds a CRITICAL warning about the same feedback-loop anti-pattern and a note to validate in a real project before tuning integration tests.
compileTask dependency wiring refactor
server/src/ba/sake/deder/CoreTasks.scala
Drops dependsOn references to generatedSourcesTask, compileClasspathTask, and semanticdbDirTask; adds mandatoryDependenciesTask, allDependenciesTask, and compileOnlyDependenciesTask. Execution logic derives classesDir, semanticdbDir, and generatedSourcesDir from ctx.out at runtime and rebuilds fullCompileClasspath from resolved jar lists and transitive upstream classes dirs.
Integration test for compile cache hit
integration/test/src/ba/sake/deder/CachedTaskSuite.scala
Adds a test that runs compile twice, reads newly appended server log lines after the first run, and asserts the second run produces a cache hit without recomputing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • sake92/deder#81: Directly modifies compileTask caching and execution behavior in CoreTasks.scala, overlapping with this PR's changes to the same function.
  • sake92/deder#73: Modifies allDependenciesTask whose output is now a direct input to compileTask's refactored dependency graph.
  • sake92/deder#54: Changes compileTask's result type and compilation summarization in the same function modified here.

Poem

🐇 Hop hop, the cache was blind,
Hashing outputs of its own kind!
Now paths flow from ctx.out at run,
No feedback loop — the cache has won!
Two compiles run, the second skips free,
A bunny grins at efficiency. 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix/compile cache self reference' directly and clearly describes the main change: fixing a self-referential caching issue in the compile task.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/compile-cache-self-reference

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/content/reference/caching.md`:
- Around line 99-100: The tracked-offender note in the caching.md file at lines
99-100 describes the compileTask dependencies and self-referential classes
wiring as current issues, but the implementation has since removed these
dependencies in CoreTasks.scala. Reword this note to present it as historical
context rather than an active offender, making clear that this issue has been
resolved. Additionally, update the corresponding wording at line 65 in AGENTS.md
to align with this same historical framing, ensuring both documents consistently
reflect that these are no longer current implementation problems.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c7966df-06c9-493e-9fbb-a35092197c20

📥 Commits

Reviewing files that changed from the base of the PR and between 123dca5 and d3783c5.

⛔ Files ignored due to path filters (1)
  • deder.pkl is excluded by !**/*.pkl
📒 Files selected for processing (4)
  • AGENTS.md
  • docs/content/reference/caching.md
  • integration/test/src/ba/sake/deder/CachedTaskSuite.scala
  • server/src/ba/sake/deder/CoreTasks.scala

Comment on lines +99 to +100
> **Tracked offenders (2026-06-14):** `compileTask` depends on `semanticdbDirTask`, and (via `compileClasspathTask → allClassesDirsTask`) on its own `classesTask` — both directories `compile` writes. Until they're removed from `compile`'s cache key (derive the paths at execute time instead), `compile` re-runs Zinc and re-hashes its class tree on every no-op.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the tracked-offender note; it no longer matches the implementation.

Line 99 still says compileTask currently depends on semanticdbDirTask / self-referential classes wiring, but this PR’s server/src/ba/sake/deder/CoreTasks.scala removes those deps. Please reword this as historical context (or remove the block), and align AGENTS.md Line 65 wording with that update.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/content/reference/caching.md` around lines 99 - 100, The
tracked-offender note in the caching.md file at lines 99-100 describes the
compileTask dependencies and self-referential classes wiring as current issues,
but the implementation has since removed these dependencies in CoreTasks.scala.
Reword this note to present it as historical context rather than an active
offender, making clear that this issue has been resolved. Additionally, update
the corresponding wording at line 65 in AGENTS.md to align with this same
historical framing, ensuring both documents consistently reflect that these are
no longer current implementation problems.

@sake92
sake92 merged commit a1a95f5 into main Jun 14, 2026
1 of 4 checks passed
@sake92
sake92 deleted the fix/compile-cache-self-reference branch June 14, 2026 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant