test(compile): lock in auto-opt freshness on runtime/stdlib source edits - #7155
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds regression coverage for content-based freshness of optimized runtime and standard-library archives. Identical-byte rewrites preserve fingerprints, while source edits rotate fingerprints and invalidate archives built from the previous source state. ChangesOptimized library freshness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
975db96 to
75d7979
Compare
The auto-optimize build stamp is keyed on a content fingerprint of every source tree that lands in the runtime/stdlib archives (added in PerryTS#5930 to close PerryTS#5892), so a source edit invalidates a cached target/perry-auto-<hash> archive even when mtimes lie (git checkout, cp -p, CI cache restore). That guarantee had direct test coverage only for the routed perry-ext-* crates, not for perry-runtime / perry-stdlib themselves -- the crates a runtime dev edits most, and the ones behind the GC-iteration "perry compile silently reused a stale libperry_runtime.a" trap. Add two regression tests: - source_fingerprint_tracks_runtime_and_stdlib_content_not_mtimes: a content edit to perry-runtime or perry-stdlib rotates the fingerprint, while rewriting identical bytes does not (keeps the no-edit rebuild a fast no-op). - runtime_source_edit_rotates_build_stamp_and_fails_freshness: ties it to the gate the driver consults -- a runtime edit rotates the build stamp so archives stamped for the pre-edit source fail auto_optimized_archives_are_fresh, no manual rm needed.
Both tests added by this PR passed with their subject sabotaged, which is the "a gate must assert its subject was live" trap CLAUDE.md documents. Measured before this commit: - Deleting the build-stamp comparison in `auto_optimized_archives_are_fresh` (`Ok(stamp) if stamp == expected_build_stamp` -> `Ok(_)`) left `runtime_source_edit_rotates_build_stamp_and_fails_freshness` GREEN. The test planted the archives BEFORE the source edit, so the edited file's own mtime was newer than the archives and the mtime half of the gate rejected them on its own — the stamp never had to be consulted. - Making the source fingerprint content-blind (hash the path, skip `fs::read`) left the same test GREEN, because its "content edit" created a NEW file (`crates/perry-runtime/src/gc.rs`) and a path-set-only hash still rotates when a path is added. Two surgical changes, no new assertions: - Both tests now edit an EXISTING runtime source file in place (`crates/perry-runtime/src/lib.rs`) instead of adding `gc.rs`, so the assertion rides on the file-content hash, not on the path set. - `runtime_source_edit_rotates_build_stamp_and_fails_freshness` plants the archives + stamp AFTER the edit, so every source is older than them and the mtime half votes "fresh". The stamp mismatch is then the only thing that can answer "stale" — which is also the real scenario the content fingerprint exists for (git checkout / cp -p / CI cache restore hand back sources whose mtimes never advance past a cached archive). Verified red-then-green: with either sabotage applied both tests now FAIL; with the tree clean `cargo test --bins -p perry optimized_libs` is 28/28.
75d7979 to
d6ce397
Compare
2 participants
Someone editing the Perry runtime or standard library needs
perry compileto pick up that edit. The failure mode this guards against is the worst kind: the compile succeeds, prints nothing unusual, and produces a binary built from the code as it was before the edit. Anyone debugging in that state is reading output from a build that does not match their source, and the only way out is knowing to delete a cached archive by hand.That guarantee already holds on
main. This pull request is test coverage only, so that it cannot quietly stop holding. It adds two regression tests and changes no product code.Why staleness is possible at all, and how it is already prevented
perry compilecaches compiled archives of the runtime and standard library undertarget/perry-auto-<hash>/release/, so that a normal compile does not rebuild them every time. Deciding whether a cached archive is still usable is the freshness gate's job.The obvious way to decide is to compare modification times: if the source is newer than the archive, rebuild. That test is wrong more often than it looks, because modification times can lie. A
git checkout, acp -p, or a continuous-integration cache restore can all hand back source files whose timestamps are older than a cached archive built from different content.The gate therefore keys the build stamp on a content fingerprint of every source tree that ends up in the archives, computed by
auto_optimized_source_fingerprint. That function was added in #5930 to close #5892. Because the fingerprint is over content rather than timestamps, a source edit invalidates the cache even when the timestamps say otherwise.Confirming the reported symptom no longer reproduces
The report was that after editing
crates/perry-runtime,perry compileproduced a binary from the old runtime, and the only workaround was to manuallyrmthe cached archive. That was hit during garbage-collector iteration work.Checked on
origin/mainwith a prebuiltperrybinary, which is after #5930 landed. The bug does not reproduce:crates/perry-runtime/src/gc/and runningperry compileprintsauto-optimize: rebuilding runtime+stdliband rebuilds the archive, with no manualrmneeded.The coverage gap this closes
The correctness fix is already in place; what was missing was a test that pins it for the runtime and standard-library crates specifically. Those are the crates a runtime developer edits most often, and they were the least covered.
The existing fingerprint tests only exercised the routed
perry-ext-*crates. The one test that did touch the runtime,..._are_stale_when_runtime_source_is_newer, exercises the modification-time half of the gate rather than the content fingerprint. So the property that actually saved the reported case had no direct test behind it.The two new tests
source_fingerprint_tracks_runtime_and_stdlib_content_not_mtimesasserts that a content edit to eitherperry-runtimeorperry-stdlibrotates the fingerprint, and that rewriting byte-identical content does not. The second half is what keeps the no-edit fast path honest.runtime_source_edit_rotates_build_stamp_and_fails_freshnessasserts that a runtime edit rotates the build stamp, so that archives stamped for the pre-edit source correctly failauto_optimized_archives_are_fresh.cargo test -p perry --bins optimized_libsreports 25 passed and 0 failed.Note on the performance trade-off
The fingerprint reads the runtime and standard-library source content on every compile, and it is deliberately not cached by modification time. A cache keyed on modification times would reintroduce precisely the cache-restore staleness that #5930 fixed.
The read is small relative to the cost of a compile, and the no-edit path remains a fast no-op, which the new tests assert.