Check the pks version before taking the cache's stat fast path - #69
Merged
Merged
Conversation
c2fe84f (#66) added the pks version to each cache entry's content digest so that an upgrade would invalidate every entry. But the stat fast path from 96f7fb0 (#59) returns an entry as soon as its file's mtime and length match, without comparing the digest. So for any file that hadn't changed, an entry written by another version of pks was still served, and the CHANGELOG's "Upgrading pks invalidates cached results" didn't hold for any entry that recorded a stat. The fast path now also requires the entry's digest to end with this version's suffix. An entry from another version falls through to the digest comparison, misses, and is rewritten. The suffix is one constant, used both to build the digest and to check it, so the two can't drift apart. Neither change has shipped. v0.5.0 predates both, and its entries have no stat, so they already miss on the digest. The bug affects builds of main since #59, and would have affected every upgrade starting from the next release. The CHANGELOG entry already describes the fixed behavior, so it's unchanged. test_entry_from_another_pks_version_is_a_miss now also covers an entry carrying the file's real stat. Its comment said a real stat would take the fast path instead, which was the bug. A new integration test rewrites every entry as if another version had written it, with its references removed and its stat still matching, and checks that the violations are still reported. Both tests fail without the fix.
The fast-path comment said a matching stat meant the file "cannot have changed in any way we care about", which the version paragraph right below it contradicts: the file is unchanged, but its entry can still be stale. The slow-path comment now lists an entry from another version among the reasons for getting there. The unit test now asserts that the file has a usable stat. On a filesystem with whole-second mtimes it has none, and the loop's second pass would quietly repeat the first instead of reaching the fast path. The integration test strips the version with split_once, since an md5 hex digest has no hyphens and a prerelease version can.
7 tasks done
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.
Summary
#66 added the pks version to each cache entry's content digest, so that an upgrade would invalidate every entry. But the stat fast path from #59 returns an entry as soon as its file's mtime and length match, and never compares the digest. So for any file that hadn't changed, an entry written by another version of pks was still served. The Unreleased CHANGELOG entry "Upgrading pks invalidates cached results" didn't hold for any entry that recorded a stat.
The fast path now also requires the entry's digest to end with this version's suffix. An entry from another version falls through to the digest comparison, misses, and is rewritten. The suffix is now one constant,
DIGEST_VERSION_SUFFIX, used both to build the digest and to check it.Who this affects
Nothing released has the bug. v0.5.0 predates both #59 and #66, and its entries have no stat, so they already miss on the digest. It affects builds of main since #59, and without this fix it would affect every upgrade starting from the next release. The CHANGELOG entry already describes the fixed behavior, so it's unchanged.
To reproduce on main, run a build, change only the version, and run again on the same untouched files. Results come from the old cache entries until a file is touched or
--no-cacheis passed.Test plan
test_entry_from_another_pks_version_is_a_missnow covers an entry carrying the file's real stat as well as one with no stat. Its old comment said a real stat would take the fast path instead, which was the bug.test_entry_from_another_pks_version_is_not_served_on_the_fast_pathrewrites every cache entry as if another version had written it, with its references removed and its stat still matching the file, and checks thatpks checkstill reports the same violations.cargo test(315 passed), fmt, and clippy with-Dwarnings.