perf(mutatediff): cache clean per-package mutation results - #923
Merged
Conversation
The diff-scoped gate had no memory: every invocation re-ran every package in the branch diff, so amending a one-line fix on a 22-package branch re-mutated all 22 at a full-package gremlins run plus that package's whole test suite each. scripts/mutatediff/cache.go adds a gitignored result cache under .mutatediff-cache/. A package is skipped only when a previous run judged the same lines, over identical inputs, entirely clean. Correctness rules, all of them fail-open: - PASS only, and only a *fully* clean pass. A survivor, a NOT COVERED or TIMED OUT mutant, and a vacuous package are all refused. Timeouts are load-dependent, so caching one would let a machine under load mint a permanent pass for a mutant that never ran. Refusing every non-clean verdict also means a hit contributes nothing to the report, so the final verdict lines read exactly as they would without the cache. - The key covers every input that can change a verdict: the package, the content of every .go file (test files included) and testdata/ in the subtree the engine mutates AND in its transitive first-party dependency closure (from `go list -deps -test`), the pinned engine command with its @Version, .gremlins.yaml, the module pins, the Go toolchain, GOOS/GOARCH, and the caller's GOFLAGS (which can carry -tags). - The judged line set is compared, not hashed. A cached pass is evidence only about the lines it judged, so an exact match or a subset hits and one extra line misses. - Every read error, parse error, schema mismatch, clock anomaly, or unresolvable dependency is a miss. - Entries carry a schema version and are ignored when it differs. Deliberately excluded from the key: worker count, CPU budget, and the timeout ceiling. Those move timings only, and any run with a timing-sensitive verdict is never stored in the first place. `-no-cache` (Makefile: MUTATE_NO_CACHE=1) bypasses it; `make clean` removes it. Every skip prints `mutatediff: <pkg> cached PASS (skipped)`, so a cached run is never mistakable for one that did the work.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe mutation runner caches clean package results. Cache keys include source, dependency, configuration, toolchain, platform, and flag data. The runner validates changed-line coverage and supports cache bypass through ChangesMutation result cache
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run
participant gateRun
participant resultCache
participant mutationEngine
run->>gateRun: start mutation run
gateRun->>resultCache: lookup package result
resultCache-->>gateRun: cached clean result or cache miss
gateRun->>mutationEngine: mutate uncached package
mutationEngine-->>gateRun: mutation verdicts
gateRun->>resultCache: store clean non-vacuous result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
Known limitations of the cache keyRecording these here because they are the paths by which a cache could go stale
Reviewer noteThe 32.3s → 0.60s warm-run figures were measured by the agent that wrote this, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/mutatediff/cache.go`:
- Around line 313-318: Update the go list execution in the result-cache flow to
capture stderr alongside the command output and include it in the error returned
when the command fails. Preserve the existing wrapped context in the error so
newResultCache’s WARN log exposes the underlying go list diagnostics.
- Around line 116-126: Update repoRoot to resolve the repository’s actual
top-level directory via git rev-parse --show-toplevel before cache creation,
rather than treating the current working directory as the root; preserve the
existing error-wrapping behavior and return an error when the command cannot
identify a repository.
🪄 Autofix
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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 994df58d-f69c-44d9-a641-e90ebef5c48e
📒 Files selected for processing (7)
Makefilescripts/mutatediff/cache.goscripts/mutatediff/cache_test.goscripts/mutatediff/diff.goscripts/mutatediff/main.goscripts/mutatediff/main_test.gowiki/testing.md
The result cache took its root from the working directory, while the changed-line map it is keyed against comes from `git diff`, whose paths are repo-root relative. An invocation from a subdirectory put package dirs and changed files in two different path spaces, and scattered one cache directory per directory the tool ran from. repoRoot now asks git for the top level, reusing main.go's gitOutput helper: git is already a hard prerequisite, since run() computes the diff before it builds the cache, so this adds no new dependency. loadPackageGraph now captures `go list` stderr into its error. Output stashes stderr on *exec.ExitError, but ExitError.Error prints only "exit status 1", so the WARN that reports the cache disabling itself carried no diagnostic at all. writeModule git-inits its throwaway module, which is what keeps TestResultCacheDisablesItselfOnAnUnlistableTree honest: without a resolvable root every cache there would be nil before ever reaching the `go list` seam that test claims to pin. It also stops a TMPDIR nested in a real checkout from resolving to that checkout instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
mutatediffhad no memory: every invocation re-ran every package in the branchdiff, so amending one line on a 22-package branch re-mutated all 22. It now caches
clean per-package results, keyed on the package's own
.gofiles (tests included),its transitive first-party dependency closure,
testdata/, the pinned enginecommand,
.gremlins.yaml, module files, toolchain/GOFLAGS, and a schema version.Impact
None for consumers — dev tooling only.
make mutategains a cache under agitignored path;
MUTATE_NO_CACHE=1opts out. A cached skip prints its own line,so a warm run is never mistaken for one that did the work.
Verification
Only clean results are stored: survivors,
NOT COVERED, andTIMED OUTare nevercached, so a load-induced timeout cannot mint a permanent pass. A cached pass is
reused only when every line the current run would judge was judged before —
stored and compared, not hashed — so a widened range misses.
Summary by CodeRabbit
New Features
Documentation
Chores